8000 Fix Xiaomi merged packet parsing by Alex9779 · Pull Request #1293 · esphome/esphome · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix Xiaomi merged packet parsing #1293

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 13, 2020
Merged

Fix Xiaomi merged packet parsing #1293

merged 5 commits into from
Oct 13, 2020

Conversation

Alex9779
Copy link
Contributor

Description:

fixes parsing of Xiaomi merged packets

Related issue (if applicable): fixes esphome/issues#1500

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

ahpohl
ahpohl suggested changes Sep 22, 2020
View reviewed changes
@ahpohl
Copy link
Contributor
ahpohl commented Sep 23, 2020 •
edited
Loading

@Alex9779 I improved the while loop slightly and the new code works for me, but I do not have a sensor which broadcasts multiple values at once like the CGG1, hence I cannot fully test the reporting of multiple values. You can download my changes from here: https://github.com/ahpohl/esphome/tree/multi_values.

I do not fully understand how the multi values are actually being reported. parse_xiaomi_value fills the XiaomiParseResult results structure, which gets overwritten as soon as parse_xiaomi_value is called again for the next value. report_xiaomi_results is only called once per service data so it would report only the last value in my understanding. Could you explain this?

@Alex9779
Copy link
Contributor Author
Alex9779 commented Sep 23, 2020 •
edited
Loading

I saw you branch this morning looked over it bit didn't have time to really think through and answer.
My code works for me and since I use it on my devices I get all data again from the devices which do send those combined messages. Not all of my CGG1s do this.
Regarding your question how this works I am not the best C/C++ programmer but you pass just an address of a reference to the result and if it is a different type of message then a different property is written...

8000
@Alex9779
Copy link
Contributor Author

I reviewed your code, I could rename the variables, but I am still not sure about the checks, not mine and not yours actually, when entering the loop and just before...

So what do you expect now? Shall I take some changes and commit them to my PR? Shall I withdraw and you create a new one?

@ahpohl
Copy link
Contributor
ahpohl commented Sep 23, 2020

Most sensors will cleanly process the payload and will not enter the loop again when all data is processed (payload_length = 0). But for those which append some other data the checks are necessary and make sense (esp. checking for the fixed byte 0x10).

Shall I take some changes and commit them to my PR? Shall I withdraw and you create a new one?

I have PR #1291 still pending. If you agree, I could easily extend it to include issue esphome/issues#1500.

@Alex9779 Alex9779 force-pushed the fix_xiaomi_merged_packets branch from 7e1a5b0 to aea9c3a Compare September 23, 2020 18:25
@ahpohl
Copy link
Contributor
ahpohl commented Sep 23, 2020

I do not fully understand how the multi values are actually being reported.

I checked the multi value reporting with the dummy payload 50.30.47.03.5C.DB.52.10.34.2D.58.06.10.02.EC.01.0A.10.01.5B from CGG1. You cannot report the same value twice. But it is perfectly fine to report a temperature together with a battery level, for example. report_xiaomi_results is called only once and sends all the values which are populated (actually the value are sent by the device class itself, but the same applies here as well).

This answers my last question and I am fine to go ahead with either of our code versions.

@Alex9779
Copy link
Contributor Author

But it is perfectly fine to report a temperature together with a battery level, for example.

Yeah and that is what is happening. When initially discovered it was just the battery level which is appended, there are no other reports yet but we decided to make it robust so it will be future proof if there might be other devices doing this with several different types of values.

I updated my code with your changes to values and length checking, test were successful and I still get the battery level from the devices which use that.

I just have one doubt regarding the initial check in the loop, this will be only reached if you have that residual data with encrypted payloads and not for normal devices. So on a normal parse I will never see that "value parsing is complete". Maybe a different message might make more sense?

@Alex9779
Copy link
Contributor Author

Oh btw:

I have PR #1291 still pending. If you agree, I could easily extend it to include issue esphome/issues#1500.

I don't think we should mix those two topics... :D

ahpohl
ahpohl suggested changes Sep 23, 2020
View reviewed changes
ahpohl
ahpohl approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor
@ahpohl ahpohl left a comment

Choose a reason for hiding this comment

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

Looks good. I think that's it.

@ahpohl ahpohl mentioned this pull request Sep 24, 2020
Closed
@glmnet glmnet added this to the 1.15.3 milestone Oct 13, 2020
@glmnet glmnet merged commit 0c87a9a into esphome:dev Oct 13, 2020
glmnet pushed a commit that referenced this pull request Oct 23, 2020
@Alex9779 @glmnet
Fix Xiaomi merged packet parsing (#1293)
540c620 
* Fix Xiaomi merged packet parsing

solves #1500

* renamed variables and updated payload and value checking

* renamed function and parameter

* add function to header

* changed log message
This was referenced Oct 23, 2020
Merged
Merged
sashao pushed a commit to sashao/esphome that referenced this pull request Oct 23, 2020
@Alex9779 @sashao
Fix Xiaomi merged packet parsing (esphome#1293)
ddd776a 
* Fix Xiaomi merged packet parsing

solves esphome#1500

* renamed variables and updated payload and value checking

* renamed function and parameter

* add function to header

* changed log message
This was referenced Jan 7, 2021
Merged
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
@Alex9779 Alex9779 deleted the fix_xiaomi_merged_packets branch October 1, 2021 06:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ahpohl ahpohl ahpohl approved these changes

Assignees
No one assigned
Labels
cherry-picked integration: xiaomi_ble
Projects
None yet
Milestone
1.15.3
Development

Successfully merging this pull request may close these issues.

Xiaomi BLE lost ability to get battery from CCG1 (specific) model sending combined data packets
3 participants
@Alex9779 @ahpohl @glmnet
0