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

Change somfy_iohc to recognize more messages #2258

Merged
merged 2 commits into from
Jan 22, 2023
Merged

Change somfy_iohc to recognize more messages #2258

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15789b6
Change somfy_iohc to recognize more messages
mtdcr Dec 1, 2022
8dfd7d5
Add back use of MIN in somfy_iohc_decode
mtdcr Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
116 changes: 90 additions & 26 deletions src/devices/somfy_iohc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,60 +91,122 @@ Example packets:

#include "decoder.h"

struct iohc_msg {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

/* Mandatory fields */
/* Control byte 1 */
unsigned end_flag : 1;
unsigned start_flag : 1;
unsigned protocol_mode : 1;
unsigned frame_length : 5;
/* Control byte 2 */
unsigned use_beacon : 1;
unsigned is_routed : 1;
unsigned low_power_mode : 1;
unsigned protocol_version : 3;
/* Addresses */
unsigned dst_addr : 24;
unsigned src_addr : 24;
/* Command ID */
unsigned cmd_id : 8;
char data[31 * 2 + 1]; /* variable length, converted to hex string */
unsigned crc : 16;

/* optional fields */
int seq_nr;
char mac[13];
};

static int somfy_iohc_decode(r_device *decoder, bitbuffer_t *bitbuffer)
{
uint8_t const preamble_pattern[] = {0x57, 0xfd, 0x99};
struct iohc_msg msg = {0};

uint8_t b[19 + 15]; // 19 byte + up 15 byte payload
uint8_t b[1 + 31 + 2]; // Length, payload, CRC

if (bitbuffer->num_rows != 1)
return DECODE_ABORT_EARLY;

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])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use >= to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safe from what?

Copy link
Collaborator

Choose a reason for hiding this comment

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

API changes. bitbuffer_search() does not guarantee any particular return value.

return DECODE_ABORT_EARLY;

offset += 24;

int num_bits = bitbuffer->bits_per_row[0] - offset;
if (num_bits <= 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check can never be true, remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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".

return DECODE_ABORT_EARLY;

unsigned num_bits = bitbuffer->bits_per_row[0] - offset;
num_bits = MIN(num_bits, sizeof (b) * 8);
num_bits = MIN((size_t)num_bits, sizeof (b) * 8);

int len = extract_bytes_uart(bitbuffer->bb[0], offset, num_bits, b);
if (len < 19)
if (len < 11)
return DECODE_ABORT_LENGTH;

if ((b[0] & 0xf0) != 0xf0)
return DECODE_ABORT_EARLY;

int msg_len = b[0] & 0xf; // should be 6 or 8
if (len < 19 + msg_len)
msg.frame_length = b[0] & 0x1f;
if (len < msg.frame_length + 3)
return DECODE_ABORT_LENGTH;
len = msg.frame_length + 3;

msg.end_flag = (b[0] & 0x80) >> 7;
msg.start_flag = (b[0] & 0x40) >> 6;
msg.protocol_mode = (b[0] & 0x20) >> 5;
msg.use_beacon = (b[1] & 0x80) >> 7;
msg.is_routed = (b[1] & 0x40) >> 6;
msg.low_power_mode = (b[1] & 0x20) >> 5;
msg.protocol_version = b[1] & 0x03;
msg.dst_addr = (b[2] << 16) | (b[3] << 8) | b[4];
msg.src_addr = (b[5] << 16) | (b[6] << 8) | b[7];
msg.cmd_id = b[8];

unsigned int data_length = msg.frame_length - 8;
if (msg.protocol_mode == 0 || data_length < 8) {
bitrow_snprint(&b[9], data_length * 8, msg.data, sizeof msg.data);
} else {
data_length -= 8;
bitrow_snprint(&b[9], data_length * 8, msg.data, sizeof msg.data);
msg.seq_nr = (b[9 + data_length] << 8) | b[9 + data_length + 1];
bitrow_snprint(&b[9 + data_length + 2], 6 * 8, msg.mac, sizeof msg.mac);
}

msg.crc = (b[len - 2] << 8) | b[len - 1];

// calculate and verify checksum
if (crc16lsb(b, len, 0x8408, 0x0000) != 0) // unreflected poly 0x1021
return DECODE_FAIL_MIC;
decoder_logf_bitrow(decoder, 2, __func__, b, len * 8, "offset %u, num_bits %u, len %d, msg_len %d", offset, num_bits, len, msg_len);

int msg_type = (b[0]);
int dst_id = ((unsigned)b[4] << 24) | (b[3] << 16) | (b[2] << 8) | (b[1]); // assume Little-Endian
int src_id = ((unsigned)b[8] << 24) | (b[7] << 16) | (b[6] << 8) | (b[5]); // assume Little-Endian
int counter = (b[len - 10] << 8) | (b[len - 9]);
char msg_str[15 * 2 + 1];
bitrow_snprint(&b[9], msg_len * 8, msg_str, 15 * 2 + 1);
char mac_str[13];
bitrow_snprint(&b[len - 8], 6 * 8, mac_str, 13);
decoder_logf_bitrow(decoder, 2, __func__, b, len * 8, "offset %u, num_bits %u, len %d, msg_len %d", offset, num_bits, len, msg.frame_length);

/* clang-format off */
data_t *data = data_make(
"model", "", DATA_STRING, "Somfy-IOHC",
"id", "", DATA_FORMAT, "%08x", DATA_INT, src_id,
"dst_id", "Dest ID", DATA_FORMAT, "%08x", DATA_INT, dst_id,
"msg_type", "Msg type", DATA_FORMAT, "%02x", DATA_INT, msg_type,
"msg", "Message", DATA_STRING, msg_str,
"counter", "Counter", DATA_INT, counter,
"mac", "MAC", DATA_STRING, mac_str,
"id", "Source", DATA_FORMAT, "%06x", DATA_INT, msg.src_addr,
"dst_id", "Target", DATA_FORMAT, "%06x", DATA_INT, msg.dst_addr,
"msg_type", "Command", DATA_FORMAT, "%02x", DATA_INT, msg.cmd_id,
"msg", "Message", DATA_STRING, msg.data,
"mic", "Integrity", DATA_STRING, "CRC",
"mode", "Mode", DATA_STRING, msg.protocol_mode ? "One-way" : "Two-way",
"version", "Version", DATA_INT, msg.protocol_version,
NULL);

if (msg.protocol_mode == 1)
data = data_append(data,
"counter", "Counter", DATA_INT, msg.seq_nr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that and thought it would obfuscate the code. Therefore I chose to avoid it.

"mac", "MAC", DATA_STRING, msg.mac,
NULL);
/* clang-format on */

if (decoder->verbose) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a good idea to have decoder data depended on verbosity level.
If this is just debug output please use decoder_logf()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

data_t *flags = data_make(
"end", "End", DATA_INT, msg.end_flag,
"start", "Start", DATA_INT, msg.start_flag,
"mode", "Mode", DATA_INT, msg.protocol_mode,
"beacon", "Beacon", DATA_INT, msg.use_beacon,
"routed", "Routed", DATA_INT, msg.is_routed,
"lpm", "LPM", DATA_INT, msg.low_power_mode,
NULL);
data = data_append(data, "flags", "Flags", DATA_DATA, flags, NULL);
}

decoder_output_data(decoder, data);
return 1;
}
Expand All @@ -158,6 +220,8 @@ static char *output_fields[] = {
"counter",
"mac",
"mic",
"mode",
"version",
NULL,
};

Expand Down