Add F2600 support#35
Conversation
flip-dots
left a comment
There was a problem hiding this comment.
So this is pretty good, just a few minor things.
The whole get_status_update thing is not very well thought out and that is my fault, I added it as a "this is kind of neat but I don't have a use for it" type of thing and did not look into it any further and left it kind of half arsed. It's something I will probably address eventually but these kinds of architectural changes will need to wait until I have more time when my dissertation and exams are over which will be in a couple of months.
Since this is a fairly simple PR which does not require much time on my part if you just fix the mentioned issues from the comments I am happy to merge it and deal with the bigger architectural changes later, though I might not have the time to add it to the Home Assistant integration for a while.
|
I've done some more testing and found some discrepancies from the F2000 implementation:
I am not sure what the difference is between AC_POWER_OUT and AC_POWER_OUT_SOCKETS is, since these models only have AC sockets. |
|
Hi, This looks fine but I don't currently have the time to do a proper review and release, I still have a few weeks of exams left but I should be able to sort this out after that, sorry :( |
|
So I have finally had the time to do a review and it looks good but it could use some tests. I would like to see some tests for the values like the ones here and maybe some for the commands like the ones here since I plan on doing quite a bit of refactoring soon and would rather not accidentally break this stuff since I don’t have this device to test with. They don't need to be perfect since the existing tests are in need of some major refactoring but it would be good to have something. You can get test coverage using pytest --cov which should help guide you on anything you might have missed. I am not going to require you to do it, but it would also be great to have a test added for the negotiation using data captured from your device since these do vary across devices and it would help me in future if I ever get around to properly figuring out how negotiation works. Other than all that this looks great, happy to merge once the merge conflict is resolved and some tests are added. |
Add F2600 support as discussed in #34
Tested on my own F2600 to the best of my ability.
It seems that some parameters like AC charge power and display timeout seconds are only returned during a full "get_status_update" response. Currently the
_process_telemetryfunction will overwrite_dataon every call, so if there is a response from the battery that does not include everything, all other parameters will be removed. While I was doing my testing I made a change to update parameters instead of a full overwrite, if you think that is useful it can be found here: https://github.com/unex/SolixBLE/tree/update-parameters