Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major update for Android runtime support and camera devices. #116

Closed
wants to merge 2 commits into from

Conversation

koush
Copy link

@koush koush commented Nov 27, 2020

Pull Request Checklist

Please confirm that you've done the following when opening a new pull request:

  • [X ] For fixes and other improvements, please reference the GitHub issue that your change addresses.
  • [X ] For fixes, optimizations and new features, please add an entry to the CHANGES.md file.
  • [X ] Run mvn compile before committing, so that the auto-code formatter will format your changes consistently with the rest of the project.

@koush
Copy link
Author

koush commented Nov 27, 2020

I had forked this repository a couple years ago and never contributed back. I basically hand picked all my changes�/fixes and cleaned them up to apply them to this repo (which had a large refactor and package rename).

I'll leave comments inline.

Note that this just exposes the camera services, configurations, and makes it easy to respond with camera snapshots (just a byte array of a jpeg). It's up to the implementer to parse the TLV and actually manage the setupEndpoints and selectedRTP characteristic setters. I use FFMPEG, similar to Homebridge.

@koush koush force-pushed the master branch 15 times, most recently from 09897d5 to 879c9d1 Compare November 28, 2020 07:01
@koush
Copy link
Author

koush commented Nov 28, 2020

Unclear why travis is failing. I think the gradle wrapper is correct and working properly on a clean checkout from my machine.

@koush
Copy link
Author

koush commented Nov 28, 2020

Fixes this. #117

@koush koush force-pushed the master branch 5 times, most recently from 58feab6 to 66d0b75 Compare December 2, 2020 19:08
@yfre
Copy link
Contributor

yfre commented Dec 8, 2020

@koush thank you for PRs.
it has many very good things, e.g.

maybe you could it split in several PRs so that it is easier to review and easier to get CI working

@koush
Copy link
Author

koush commented Dec 9, 2020

I doubt I'll have the time to break this up into separate changes, as I had to basically implement it as one big patch to simply get everything working again (due to the package rename). The old cherry picks are for the pre-rename code.

I think the reason CI is failing is because it detects the build.gradle which is necessary for compiling+debugging on android. I think deleting that file would fix the CI. The CI needs to ignore it. It may be possible to delete it, but then Android would not compile. the mvn built jars should work on Android too though.

The patch is fairly non-intrusive and actually not that large. The majority of the files are the gradle wrapper and the various data types for video (which are unused anyways).

@koush koush closed this Dec 9, 2020
@koush koush reopened this Dec 9, 2020
@gjvanderheiden
Copy link
Contributor

I agree this could use a split up.

BouncyCastle could be updated to 1.67, with a small modification
if (!Arrays.constantTimeAreEqual(calculatedMAC, receivedMAC)) {
throw new TlsFatalAlert(AlertDescription.bad_record_mac);
}
TlsFatalAlert could be converted into IOException, because TlsFatalAlert doesn't exist anymore in .67. TlsFatalAlert is just an IOException, no biggie.

I also was a bit surprised there is no TLV object in HAP-java. You introduced it, but now it isn't used in the server packages.
So it is a really nice addition and should be exploited in the server package with some extra methods (getState(), getMethod() etc).

@gjvanderheiden
Copy link
Contributor

Could you maybe rename the gradle file for now, so that the build doesn't fail?

@@ -113,7 +113,7 @@ public BaseCharacteristic(
.add("type", shortType)
.add("perms", perms.build())
.add("format", format)
.add("ev", false)
// .add("ev", false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it your intention to comment this out? Looks like a rather common piece of code.

@@ -101,19 +101,19 @@
<dependency>
<groupId>com.nimbusds</groupId>
<artifactId>srp6a</artifactId>
<version>1.5.2</version>
<version>2.0.2</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.1.0 is the newest

</dependency>

<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk15on</artifactId>
<version>1.51</version>
<version>1.60</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.67 is the newest

@@ -32,7 +32,7 @@ Current implementation fully supports 37 HomeKit accessory/services.
| Air Quality Sensor | :white_check_mark: |
| Audio Stream Management | :x: |
| Battery Service | :white_check_mark: |
| Camera RTP Stream Management | :x: |
| Camera RTP Stream Management | :white_check_mark: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs update from hap-java/hap-java

And this text needs update:
Current implementation fully supports 37 HomeKit accessory/services.

@@ -27,3 +27,5 @@
* Occupancy sensor support [#59](https://github.com/hap-java/HAP-Java/pull/59)
* Leak sensors and valve support added [#52](https://github.com/hap-java/HAP-Java/pull/52)
* Notifications are batched now, when possible [#66](https://github.com/hap-java/HAP-Java/pull/66)
# Android Support
# Camera Support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be an asterisk instead of a hash

@@ -27,3 +27,5 @@
* Occupancy sensor support [#59](https://github.com/hap-java/HAP-Java/pull/59)
* Leak sensors and valve support added [#52](https://github.com/hap-java/HAP-Java/pull/52)
* Notifications are batched now, when possible [#66](https://github.com/hap-java/HAP-Java/pull/66)
# Android Support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be an asterisk instead of a hash

// NOTE: The BC implementation puts 'r' after 'k'
System.arraycopy(firstBlock, 0, firstBlock, 32, 16);
KeyParameter macKey = new KeyParameter(firstBlock, 16, 32);
// NOTE: The older BC implementation puts 'r' after 'k'0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still applicable?

// NOTE: The BC implementation puts 'r' after 'k'
System.arraycopy(firstBlock, 0, firstBlock, 32, 16);
KeyParameter macKey = new KeyParameter(firstBlock, 16, 32);
// NOTE: The older BC implementation puts 'r' after 'k'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this comment still applicable?

.write(Json.createObjectBuilder().add("accessories", accessories).build());
JsonWriter writer = Json.createWriter(baos);
writer.write(Json.createObjectBuilder().add("accessories", accessories).build());
writer.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonWriter is an AutoClosable, makes more sense to use the try block:

try {(ByteArrayOutputStream baos = new ByteArrayOutputStream();
JsonWriter writer = Json.createWriter(baos))

JsonWriter writer = Json.createWriter(baos);
writer.write(
Json.createObjectBuilder().add("characteristics", characteristics.build()).build());
writer.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonWriter is an AutoClosable, makes more sense to use the try block:

try {(ByteArrayOutputStream baos = new ByteArrayOutputStream();
JsonWriter writer = Json.createWriter(baos))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants