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

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.

@flip-dots

Copy link
Copy Markdown
Owner

Yea the parsing isn’t quite as detailed as your previous implementation, and feel free to improve it, but I want to stay away from data classes and manual parsing / building and instead rely upon constructs to do the parsing / building. Using constructs means only the data structure needs to be defined which simplifies things a lot and the changes to make more use of existing functions / structure is imperfect, but at least easier for me to maintain since its less unique code and in keeping with existing design choices.

With respect to the acks, I don't currently have a good solution for those across the whole library yet, there is a _listen_for_packet() command implemented for other protocols which you could probably use to listen for them and implement some additional state for them if you want, but it might be best to leave that for later if/when a library wide system is created for them that this could use.

@impala454

Copy link
Copy Markdown
Author

Hey @flip-dots appreciate the comments. I totally get that you need maintainability from your end and that is difficult particularly when you don't own the device. I think since this protocol is so different from the mainline I would choose to just keep it separate for now until such a time that I can attempt to make it work 100% and align with your wishes for the repo. I disagree with some of the design choices but respect that it's your repo and you're maintaining it so I'll do my best to align. Unfortunately that won't be any time soon as time to work on it is thin at the moment. That said I'd suggest keeping this as a branch if you like (I will do so on my end as well) as I'll likely close this PR and revert my fork back to the my last commit.

This branch has not been deployed

No deployments
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