Change somfy_iohc to recognize more messages #2258
Conversation
- Messages can be as short as 11 bytes. - Message length has 5 bits instead of 4. - Flags were assumed to be constant, causing two-way remotes not to get recognized. - The message type (command id) is not indicated by the 1st but the 9th byte. - Trailing bytes were not removed, leading to invalid length and failing CRC. - Sequence numbers and MAC are limited to authenticated one-way frames. This was briefly tested using both one- and two-way remotes.
| unsigned offset = bitbuffer_search(bitbuffer, 0, 0, preamble_pattern, 24) + 24; | ||
| if (offset + 19 * 10 >= bitbuffer->bits_per_row[0]) | ||
| unsigned offset = bitbuffer_search(bitbuffer, 0, 0, preamble_pattern, 24); | ||
| if (offset == bitbuffer->bits_per_row[0]) |
There was a problem hiding this comment.
API changes. bitbuffer_search() does not guarantee any particular return value.
| offset += 24; | ||
|
|
||
| int num_bits = bitbuffer->bits_per_row[0] - offset; | ||
| if (num_bits <= 0) |
There was a problem hiding this comment.
This check can never be true, remove.
There was a problem hiding this comment.
I did this because of the cast to size_t below. This way a reviewer will see it's safe to cast, even without knowing the data type or value range of bits_per_row[0].
Using <= instead of < was just an optimization, because it's possible to find the preamble_pattern without following data, therefore getting num_bits==0, right?
There was a problem hiding this comment.
Ok, but bitbuffer->bits_per_row[0] - offset >= 0 is checked by excluding offset >= bitbuffer->bits_per_row[0] 3 lines above. Modern compilers will infer that and bug us with "condition is constant".
src/devices/somfy_iohc.c
Outdated
| return DECODE_ABORT_EARLY; | ||
|
|
||
| unsigned num_bits = bitbuffer->bits_per_row[0] - offset; | ||
| num_bits = MIN(num_bits, sizeof (b) * 8); | ||
| if ((size_t)num_bits > sizeof b * 8) |
There was a problem hiding this comment.
keep the parens on sizeof and add braces for the block, otherwise maybe keep MIN
There was a problem hiding this comment.
Why braces? Other single-line blocks don't have any. sizeof is an operator, but well... I'll change it back to MIN with a cast.
There was a problem hiding this comment.
Operator, yes, that's why sizeof (x) is prefered to sizeof(x). The sizeof x * y just looks sketchy at a quick glance.
Newer and changed code should preferably use braces, even for single-line blocks -- the compiler and linter will warn on mishaps, still it's just an accident waiting to happen.
|
|
||
| if (msg.protocol_mode == 1) | ||
| data = data_append(data, | ||
| "counter", "Counter", DATA_INT, msg.seq_nr, |
There was a problem hiding this comment.
You can also use DATA_COND here an put in the data_make above.
E.g.
"counter", "Counter", DATA_COND, msg.protocol_mode == 1, DATA_INT, msg.seq_nr,
There was a problem hiding this comment.
I saw that and thought it would obfuscate the code. Therefore I chose to avoid it.
| NULL); | ||
| /* clang-format on */ | ||
|
|
||
| if (decoder->verbose) { |
There was a problem hiding this comment.
Not a good idea to have decoder data depended on verbosity level.
If this is just debug output please use decoder_logf()
There was a problem hiding this comment.
It clutters the output, but some people may want to see it. According to the help screen, -vv is supposed to make decoders verbose. It didn't work for me, FWIW. Feel free to adapt it. It's not debug output, as it outputs received protocol data.
There was a problem hiding this comment.
The rule we try to follow is that data on the air should be translated as straight as possible to what is output. So we want it all. It is up to the consumer/presentation layer to select what data to use.
| @@ -91,60 +91,123 @@ Example packets: | |||
|
|
|||
| #include "decoder.h" | |||
|
|
|||
| struct iohc_msg { | |||
There was a problem hiding this comment.
Do you really need a struct? I.e. using just e.g. iohc_end_flag or msg_end_flag would be simpler to maintain.
Otherwise you will need to put a @fn on the doc-comment above.
There was a problem hiding this comment.
Using a struct is both clean and easy to initialize to zero. There's no way it would add maintenance burden per se, only if you insist on adding documentation. As you'd accept the struct members as standalone variables without documentation, adding it to the struct seems rather pointless. It's not in a shared header file anyway.
|
Great stuff, looks good. Thanks! |
|
Thanks for you review! I added another commit reintroducing the use of the MIN macro. I'll stop here, because I'm not interested in discussing style preferences of yet another project just to contribute a single patch. You can modify it to suit your needs, if you like to. |
Fair enough. The work analyzing and coding the protocol is appreciated. TBH style preferences here are mostly "same dumb simple in all decoders" -- with ~200 decoders any "smart" or "neat" will cause headache when e.g. refactoring with some regex is needed. |
|
@mtdcr / @zuckschwerdt You both should have a look at iown-homecontrol |
- Messages can be as short as 11 bytes. - Message length has 5 bits instead of 4. - Flags were assumed to be constant, causing two-way remotes not to get recognized. - The message type (command id) is not indicated by the 1st but the 9th byte. - Trailing bytes were not removed, leading to invalid length and failing CRC. - Sequence numbers and MAC are limited to authenticated one-way frames. This was briefly tested using both one- and two-way remotes.
- Messages can be as short as 11 bytes. - Message length has 5 bits instead of 4. - Flags were assumed to be constant, causing two-way remotes not to get recognized. - The message type (command id) is not indicated by the 1st but the 9th byte. - Trailing bytes were not removed, leading to invalid length and failing CRC. - Sequence numbers and MAC are limited to authenticated one-way frames. This was briefly tested using both one- and two-way remotes.
This was briefly tested using both one- and two-way remotes.