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.

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

Add support for Qingping/Cleargrass advertisements and fix support for custom format #105

Merged
merged 12 commits into from
Jan 2, 2022
Merged

Add support for Qingping/Cleargrass advertisements and fix support for custom format #105

merged 12 commits into from
Jan 2, 2022

Conversation

mvdklip
Copy link
Contributor

@mvdklip mvdklip commented Dec 13, 2021

This version adds support for Qingping devices (tested with CGDK2 Temp & RH Monitor Lite) and also fixes support for custom format advertisements. The latter case was missing a check for packets of length 30.

@JsBergbau
Copy link
Owner

Hi mvdklip,
thanks for adding support for Qingping.
Can you tell more about the fix? What settings have to be in TelinkFlasher that there are problems? I thought I tested all configurations.

Sorry that I didn't merge the pullrequest so far. I'm still considering, because it is a lot of rework which makes it harder for me to maintain the code because of changes in the flow. Especially the removing of the comments makes this very hard because with the comments I know the alternatives that worked or didn't work and debug places where to look at.

@mvdklip
Copy link
Contributor Author

mvdklip commented Dec 29, 2021 •
edited
Loading

The fix is easy and can be seen in the original commit I did for adding Qingping support. I identified there was a problem there but didn't have any of my sensors running with custom format so I marked it as a FIXME:

72ee337#diff-745ff0eea3cb9a1d2361ae0f259366b55858554b9f67ffc9e3f18a0b516f82adR611-R613

Later I changed one of my sensors to custom format for testing purposes and was able to confirm the problem. I then fixed it and only after this change the custom format was being recognized by your script.

I you ask me the current version of the script can never recognize custom format because it is simply not including packets of length 30 in the outer if statement:

72ee337#diff-745ff0eea3cb9a1d2361ae0f259366b55858554b9f67ffc9e3f18a0b516f82adL622-L625

Regarding rework - I understand it might make things less familiar to you as the original author. The intention however is to increase readability / maintainability. The comments you're referring to were no real comments but commented out code. To me (and others probably) they are only noise so removing them actually increases the readability for me. Adding actual comments which explains why something is done in a certain way may be even better but that's impossible for me as I don't know the origin of this noise.

That said, I do realize that this kind of rework makes the chance smaller that you'll be willing to accept this merge request. That's the risk I took. If you think I can make the changes in this MR better then please let me know. I'm willing to work on this a bit more if that will convince you to merge. One of the thoughts I had is that maybe the '--atc' option should actually be called '--listen' because it better expresses what it does: listening for BLE packets instead of actively connecting to the sensors. I didn't make this change because it is kind of backwards incompatible but I (or you) could ofcourse make a version which recognizes both --atc and --listen but deprecated the former as an option from the past. Just mentioning this as an example to give you some insight on my thoughts on sharing back instead of forking.

@JsBergbau
Copy link
Owner

Later I changed one of my sensors to custom format for testing purposes and was able to confirm the problem. I then fixed it and only after this change the custom format was being recognized by your script.

Can you provide a screenshot of the settings for this sensor? I thought I've tested all possible combinations to work...

I you ask me the current version of the script can never recognize custom format because it is simply not including packets of length 30 in the outer if statement:

However you're right, I now see the point. Thanks for fixing this.

Regarding rework - I understand it might make things less familiar to you as the original author. The intention however is to increase readability / maintainability. The comments you're referring to were no real comments but commented out code. To me (and others probably) they are only noise so removing them actually increases the readability for me. Adding actual comments which explains why something is done in a certain way may be even better but that's impossible for me as I don't know the origin of this noise.

I see your point. There might be indeed more noise as expected. On the other way #temperature = int(data_str[22:26],16) / 10. was an important comment, because that way works to decode temperature, but only when there are no negative values.

Another thing I've now noticed for the first time, because of a lot of moved code: This part is completly missing now, but it is important, because that way RLE compression for timestamps can be used which saves really a lot of data.

			if args.influxdb == 1:
				measurement.timestamp = int((time.time() // 10) * 10)
			else:
				measurement.timestamp = int(time.time())

So looking thoroughly through the changed code takes a lot of time. Actually I should test all pull requests with all cases, but I don't have the time for that.

One of the thoughts I had is that maybe the '--atc' option should actually be called '--listen' because it better expresses what it does: listening for BLE packets instead of actively connecting to the sensors. I didn't make this change because it is kind of backwards incompatible but I (or you) could ofcourse make a version which recognizes both --atc and --listen but deprecated the former as an option from the past. Just mentioning this as an example to give you some insight on my thoughts on sharing back instead of forking.

Thanks for sharing your thoughts. Ideally the parameter should be named --atclisten. However with quingping support this is not true anymore, and also just --atc is also not correct anymore. Backward compatibility is indeed very important, so introducing a new flag is also a good idea. I was also thinking about removing all the connection code stuff, but a lot of users still use that, so this would be not optimal.
BTW: Did you try to program quingping via BLE or do you know a project for that? I once owned one, but reverse engineering that was too complicated/time consuming for me. But this would be an interesting option like when pressing Zigbee button quing ping gives short alarm to confirm button press was successful.

What can do to get this pullrequest merged: Please include the comment #temperature = int(data_str[22:26],16) / 10. again, maybe with comment that this doesn't work for negative temperatures.
Revise the influxdb thing.
Please also add in Readme information about Qingping-Support, also what data qingping reports. Does qingping need a special firmware or at least a configuration does it do that out of the box? That are questions that should be answered.

I know readme is very long. However this helps other people to get the code running. I see so many (great?) software on github, but it is really hard to get it running because of poor documentation. To get it run you must have written the code or at least look into it, which is in my point of view a waste of time, because everybody has to do that. If the developer does that onces, a lot of people save a lot of time.

@mvdklip
Copy link
Contributor Author

mvdklip commented Jan 2, 2022 •
edited
Loading

Can you provide a screenshot of the settings for this sensor? I thought I've tested all possible combinations to work...

I don't actually run any of my sensors in Custom format. I use Atc1441 format normally, but I temporarily changed one of my sensors to Custom format for testing purposes. The only thing I did was change the Advertising type to Custom in the telink flasher:

Screenshot 2022-01-02 084350

All of this ofcourse based on the pvvx firmware.

I see your point. There might be indeed more noise as expected. On the other way #temperature = int(data_str[22:26],16) / 10. was an important comment, because that way works to decode temperature, but only when there are no negative values.

I have restored that commented out line of code for future reference.

Another thing I've now noticed for the first time, because of a lot of moved code: This part is completly missing now, but it is important, because that way RLE compression for timestamps can be used which saves really a lot of data.

			if args.influxdb == 1:
				measurement.timestamp = int((time.time() // 10) * 10)
			else:
				measurement.timestamp = int(time.time())

You're right. Sorry about that. I fixed this.

So looking thoroughly through the changed code takes a lot of time. Actually I should test all pull requests with all cases, but I don't have the time for that.

Yeah, I understand.

Thanks for sharing your thoughts. Ideally the parameter should be named --atclisten. However with quingping support this is not true anymore, and also just --atc is also not correct anymore. Backward compatibility is indeed very important, so introducing a new flag is also a good idea.

In my version (and in this PR) I have renamed this to Passive Mode to make more clear what it does. It still supports the --atc and -a arguments but the main argument is --passive now.

I was also thinking about removing all the connection code stuff, but a lot of users still use that, so this would be not optimal.

You could just drop it in a new version (5) of the script and make a release (tag) for the old versions, right? That way people would still be able to use the old version of the script if they really wanted too. I would never recommend the active connection mode anyway. It drains the batteries too much and is more subject to reception issues. Especially with pvvx firmware I've had problems with connecting to devices on low batteries which were still sending out advertisements fine.

BTW: Did you try to program quingping via BLE or do you know a project for that? I once owned one, but reverse engineering that was too complicated/time consuming for me. But this would be an interesting option like when pressing Zigbee button qingping gives short alarm to confirm button press was successful.

I was planning to run (pvvx) ATC firmware on these Qingping sensors but I accidentally bought the new 'Lite' version of the round temperature sensor which is model CGGDK2. There is no custom firmware for that model so to avoid it being useless I added the support for Qingping format packets. This way it's still useful to me as a temperature and humidity beacon. I have no plans on using it otherwise. Later I bought an older model CGG1-M which can actually run pvvx firmware and has a nice e-ink display. I plan on using that one to display the amount of and the temperature of hot water in my solar boiler so me and my family can check it in the bathroom to see if it's safe to take a shower. ;-)

What can do to get this pullrequest merged: Please include the comment #temperature = int(data_str[22:26],16) / 10. again, maybe with comment that this doesn't work for negative temperatures. Revise the influxdb thing. Please also add in Readme information about Qingping-Support, also what data qingping reports. Does qingping need a special firmware or at least a configuration does it do that out of the box? That are questions that should be answered.

Did all that in a few extra commits in this PR.

I know readme is very long. However this helps other people to get the code running. I see so many (great?) software on github, but it is really hard to get it running because of poor documentation. To get it run you must have written the code or at least look into it, which is in my point of view a waste of time, because everybody has to do that. If the developer does that onces, a lot of people save a lot of time.

Agree.

@JsBergbau
Copy link
Owner

Hi mvdklip,

wow thanks for you enormous work and especially for fixing all the typos in the Readme.

In my version (and in this PR) I have renamed this to Passive Mode to make more clear what it does. It still supports the --atc and -a arguments but the main argument is --passive now.

Thanks. That is a great idea.

You could just drop it in a new version (5) of the script and make a release (tag) for the old versions, right? That way people would still be able to use the old version of the script if they really wanted too. I would never recommend the active connection mode anyway. It drains the batteries too much and is more subject to reception issues. Especially with pvvx firmware I've had problems with connecting to devices on low batteries which were still sending out advertisements fine.

Yeah you're right. Will do that on the next change and then increase version number by one. For now your changes will be released as version 5 of the script. Do you want to make the changes here

MiTemperature2/LYWSD03MMC.py

Line 5 in 4686505

print("MiTemperature2 / ATC Thermometer version 4.0")

and in Readme that with version 5 quingping sensors are supported? I can do that as well. You did a lot of work so far. I'm really impressed.
If you like you can also remove the old connection stuff. I know that is a lot of work, especially regarding readme, so I'm fine with it doing this by myself in the future.

Especially with pvvx firmware I've had problems with connecting to devices on low batteries which were still sending out advertisements fine.

I also had this problem often. Here is the explanation for that pvvx/ATC_MiThermometer#11 (comment)

I plan on using that one to display the amount of and the temperature of hot water in my solar boiler so me and my family can check it in the bathroom to see if it's safe to take a shower. ;-)

Thats an interessting use case. Capacitor may help you a lot since you have to connect to the device to change displayed value.

Do you know these https://www.alibaba.com/product-detail/Zkong-hot-products-ble-5-0_60833472383.html devices? Electronic shelf labels are cheap, look nice and are ideal to display data. They are also programmed via bluetooth, so ideal to use from Raspberry Pi. The manafacturer didn't want to send me specs on how to program that. Instead they wanted to sell their programming system for large stores which is way too much for a few of these electronic labels. Perhaps you do know some project that reversed these labels for displaying smarthome data.

Thanks again for your work. This pullrequest can be merged now. Depeding on your answer regarding version number and perhaps removing old connection stuff, I'll merge then.

@mvdklip
Copy link
Contributor Author

mvdklip commented Jan 2, 2022 via email

@mvdklip
Copy link
Contributor Author

mvdklip commented Jan 2, 2022

Update to version 5.0 is done.

I did some research on electronic shelf labels (ESL) and found various options:

Infrared is useless to me. The Telink variant is most interesting because it also supports Bluetooth and is related to these temperature sensors. Not sure if I can find how to address it through Bluetooth tough. Supposedly there are (Chinese) apps for Android which can send the correct sequences for updating these displays.

@JsBergbau JsBergbau merged commit f7c6a48 into JsBergbau:master Jan 2, 2022
@JsBergbau
Copy link
Owner

Thanks a lot for your contribution. Changes have been merged and v5.0 has been released.
Regarding discussion of ESL Labels, which is very interesting, I've created a new issue here JsBergbau/Verschiedenes#3

You can also create a new one and copy your text from here, then you're the author. I'll then delete link above and post new one here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvdklip @JsBergbau