-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Improve Holman ws5029, Add support for AOK-5056 and correction for Emax #2419
Conversation
Thanks! |
I really like your "bits details" drawing! |
Need a little correction for the wind speed. I have errors into Domoticz because of bad value to 128.9 kmh instead of 1 kmh, |
How does Domoticz read the value? From MQTT, JSON, or CSV? For JSON and MQTT at least the type int is the same as float. |
Domoticz from json , I saw a conversion km/h to m/s by dividing the value by 3.6f, (from domoticz/hardware/Rtl433.cpp , line 236).
And I'm facing to this error lot of times, like 1 is 129 , I need to correct manually the value into Domoticz db. In parallel, I monitor the weather station value from another rtl-sdr dongle, without error from rtl_433, but error into Domoticz. So for me, I deduct that the behavior is from Domoticz , some round value not done properly, may be because of int value. |
But |
Agree, I'll try another test, to get the value from the current dongle I'm using into Domoticz, to check if it could come from it. Edit: before my test, I found 2 new wrong values, 127.1 kmh and 254.9 kmh, with respectively , 353 and 708 raw value into the Wind device in Domoticz DB. (speed = 10 * m/s ) 127 and 255, impacting also the low nibble and were properly check, strange. |
Well, more than 24 hours later, still no error, I'm monitoring the json output. I will continue to monitor. I have code data that can be used to found the good algorithm for the checksum / crc. Into the code data, the 13th byte sounds to be the xor sum from the 12th first bytes, for sure for the lower nibble and I added it into the device improvement, but not the upper nibble, and I don't know how to find it ? |
Maybe revdgst can help. |
We have seen these checksums where the overflow of the lower half is folded into the upperhalf. It seems to be a kind if Fletcher checksum. We have not yet found a way to analyze those. Yes, revdgst: if you have enough messages with only very slight changes then |
Hi, thanks for help, but unfortunately no one give me a good result except it confirmed the XOR by shift up = 0x18 = 24 means the exact number of nibbles. But still no luck with the upper nibble. In another hand, I monitored the messages for more than one full day, and no error, then it's running with Domoticz, same without errors, this means that the decoder is not so bad, acceptable for me. So from my side, it's ok to merge it. |
Hi, I have enough messages to work on this xor checksum and I found that the Upper Nibble is an xor between the upper xor and lower xor of the message with a kind of shift at the lower nibble (lfsr ?) . I was able to extract all the 256 possibilities, see the table, still working on the good algorithm to make the map. For sure the result will be something related to lfsr. xor_bytes of the 12 bytes message --> upper_result XOR ( LFSR xxx ??? of lower_result ) Here the table between the calculated XOR from the message (12 bytes) and the value of the 13th Byte checksum:
|
Great success! This can be solved with a reverse Galois algorithm then (key = 0x31).
Can you double check that? |
Oh and it looks like a broken try at LFSR -- the "F" is missing ;)
not this:
|
Good to find the algorithm ! Thanks a lot.
|
I'm updating the code to add the good checksum control and commit it very soon.
|
@zuckschwerdt what is missing from revdgst to figure out this? Besides that LGTM. This change will most likely be valid for the PWM model also. |
@ProfBoc75 this PR #947 seems to have all the details/data to complete the pwm version of the protocol. Can you look into that also? |
Yes, sure, I will check that asap. |
It's a broken L(F)SR. Not cyclic and thus exhausted after 8 bits. Or maybe just degenerate as the generator here would be 0x00 (only the key is doing the work). We could restart the the L(F)SR check each byte as a special case for gen=0x00. Or like suggested here we could xor all bytes first, then run (any) LFSR -- that could be a more general case perhaps? And @ProfBoc75 is the first to figure this sad example of algorithm out. Very nice. I guess more codes could be using this? |
Basically this is xor on all but the last message byte that instead uses a non linear mapping for the last byte. The question is if that has a good rf property regarding message validation. I cant really see any other reason then to make the message unique. By doing this last step we can have a lot of protocols that have the exact same rf parameters but uses different values for the sbox. The hardware implementation would be simple. 12 xor and then one 256 bytes lookup table. So I would not say it is a broken implementation but rater the tiniest step-up from xor. The sbox design is the tricky part so you need to generate it. Anyway I think this is a worthy addition to revdgst. I do remember we had some other decoder where we also only where able to verify 4 bits of data. |
I found PWM Samples here Holman-WH5029-samples.zip from the subject PR #947 , sounds good to start from here, since the other files are truncated , missing end of the message which contains the checksum or whatever we can call it 😄 ... to be continued ... |
@merbanan I don't think it's a deliberate design. It clears the lower bits because the key is 0x31 (just the identity bit in the lower nibble) and that we don't rotate but shift the bits off. There is no LUT, it's just a mask that shifts. |
Hi, For the PWM, it's not the same algorithm, same approach, XOR of the bytes but the last, but not the same result as for the PCM.
FYI : rtl_433 -r g*.cu8 -X "n=TEST,m=FSK_PWM,s=488,l=976,g=2000,r=6000,invert" to get the above values, I removed the last byte, not relevant, (always 00 or 80). |
I have an old test algorithm that I don't quite follow anymore, but it seems this is the same pattern but a key of 0x18? |
0x18 doesn't match, galois = lfsr_digest8_reflect(&xor, 1, 0x00, 0x18) holman_ws5029pwm_decode: 11th byte 5b xor 6b galois e8 |
Actually something more like this: Add all odd rows (indexes 0, 2, ...) to result0, add all even rows (indexes 1, 3, ...) to result1.
|
Good job ! this one is working for PWM. I will update the PWM part accordingly, and enrich also the comments that are very weak for PWM information (no bitbench, no byte position detail ... ) . Then all will be properly check, not only the lower nibble !
|
It's special for now. I'm not sure how they calculate something like this. It easy but very strange: |
I propose to call this "mic" "Integrity" method "XORSHIFT" , instead of CHECKSUM or any other suggestion before my commit ? |
We only want to indicate the approximate strength. The strength of Sum/Xor and such is "CHECKSUM". We only care if the check is significantly better (Digest/CRC) then it's "CRC". |
src/devices/holman_ws5029.c
Outdated
@@ -86,6 +87,25 @@ To get raw data | |||
|
|||
#include "decoder.h" | |||
|
|||
// see #2419 for more details about the xor_shift_bytes , used by PWM device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 14 now needs to be
/** int holman_ws5029pcm_decode(r_device *decoder, bitbuffer_t *bitbuffer)
otherwise the doc-comment above attaches to this helper function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but I moved the xor_shift_bytes function to the bottom and enrich both doc-comments for PCM and PWM.
Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, thanks. One thing: %.01f
is not a sensible format ;) It is often in our code but it should be %.1f
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was a copy paste of %.01f from the initial author, I just commit these "minor" corrections.
Hi , Please notice a little correction to the Emax device, I was facing an error with the byte conversion (for wind and rain) , 0x01 is the 0 default value. So we need to remove 1 for each byte to get the good value, but the 0x00 was not properly calculate, expected 255 and it was 65536 because of "int" is 2 bytes data. So I added the 0xff mask to get 1 byte data and the expected 255. No error since the improvement to AOK with the full checksum. So both Aok and Emax are ready to be merged. Thanks. |
src/devices/emax.c
Outdated
@@ -250,7 +250,7 @@ static int emax_decode(r_device *decoder, bitbuffer_t *bitbuffer) | |||
return ret; | |||
} | |||
|
|||
static char const *output_fields[] = { | |||
static char const *const output_fields[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already checked in. Can you rebase?
src/devices/holman_ws5029.c
Outdated
Decoder for Holman Industries WS5029 weather station, | ||
a.k.a. Holman iWeather Station. | ||
https://www.holmanindustries.com.au/products/iweather-station/ | ||
/** int holman_ws5029pcm_decode(r_device *decoder, bitbuffer_t *bitbuffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be just /**
no function link.
src/devices/holman_ws5029.c
Outdated
|
||
if (bitbuffer->num_rows != 1) { | ||
if (decoder->verbose) { | ||
fprintf(stderr, "%s: wrong number of rows (%d)\n", __func__, bitbuffer->num_rows); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always use log functions
src/devices/holman_ws5029.c
Outdated
@@ -141,29 +211,61 @@ r_device const holman_ws5029pcm = { | |||
.fields = output_fields, | |||
}; | |||
|
|||
/** | |||
/** int holman_ws5029pwm_decode(r_device *decoder, bitbuffer_t *bitbuffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function link is /** @fn int holman…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Should be better, I made few updates and did some git stuff. Let me know if it's ok for you.
Thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks very good. One more thing: log texts must not have \n
, change decoder_logf
in line 100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thx
Hi,
I propose an improvement from the current Holman ws5029 decoding to take into account the xor checksum, partially with the lower nibble but better than nothing.
And also add the support for AOK-5056, same family product with UV & Lux sensor.
My samples
BitBench