Skip to content

Add support for older F2000/767 firmware version - #64

Open
impala454 wants to merge 10 commits into
flip-dots:mainfrom
impala454:main
Open

Add support for older F2000/767 firmware version#64
impala454 wants to merge 10 commits into
flip-dots:mainfrom
impala454:main

Conversation

@impala454

Copy link
Copy Markdown

This PR adds in support for the older model F2000/767 firmware. This firmware is not upgradable to the latest and has a completely different scheme with no handshakes or negotiation. This PR adds almost all of the functionality the Anker app has to offer to this library. The only minor edit to the base class was just to allow for the different UUIDs needed and still maintain inheritance.

@flip-dots

Copy link
Copy Markdown
Owner

The new constructs in #61 should be able to make the commands and bit mangling a lot cleaner, with any luck that will be merged soon and it will be possible to take advantage of it.

@impala454

Copy link
Copy Markdown
Author

I took a look at #61 and seems like a good change but this older firmware is so completely different I don't think it would change this PR. There is no negotation, no multi part packets, and the parsing is fixed fields, so I would probably still be overriding _process_notification anyways. The only thing I needed to touch in the base was just to allow the different UUID to be recognized. I don't think that would change with #61 merged either. Let me know what you think.

@impala454

Copy link
Copy Markdown
Author

btw this closes #55

@flip-dots

Copy link
Copy Markdown
Owner

Wow..., I only skimmed this before but this is very different, its practically an entirely different protocol (I think we are now on the 4th or 5th protocol at this point). I would like to see some tests for this before I would be happy to merge it for things like sending commands and parsing telemetry. Without any tests or test data I cant properly review this or be sure that it wont be broken by future updates to this library, its especially important given that I don't have that model to physically test with.

@impala454

Copy link
Copy Markdown
Author

Totally agreed, I'll get a good suite of tests going and let you know when to look again.

@impala454

Copy link
Copy Markdown
Author

@flip-dots should be good to go now- once this is in I'll test the HA side repo for a bit w/this then get that one merged also.

@flip-dots

Copy link
Copy Markdown
Owner

I have re-written it to better fit in with the existing code structure and fixed some other issues (functions/properties with inconsistent names), could you confirm that the new code works, also any suggestions are welcome. I ended up removing the parsing of acknowledgements since they are a bit redundant given that a full status update is requested after a command is sent, feel free to have a go at re-adding them if you want though.

@impala454

Copy link
Copy Markdown
Author

Took a short look and can test later. Totally get the "legacy" rename that makes more sense. The acks are important because they only occur when the user presses the physical butttons (I thought I had explained that in the comments). Without them in there the device can get out of state when physical buttons are pressed. This is also one of the reasons I chose to implement it in a "parse on receipt" instead of "parse on demand" way. I think your parsers are fine but they're omitting a lot of the work that was done to actually figure out what the packet structure was (various parts of the header meaning different types). Instead you've turned it into matching hard coded hex values as strings which can be very brittle. Also keeping the telemetry class and parsing of the packet independent of the HA getter functions meant it was a lot more readable and editable and keeps the "business logic" independent of the external API. No worries if we don't see eye to eye just those are my initial thoughts on the changes.

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.

2 participants