Sub-IoT / Sub-IoT-Stack

Sub-IoT: Open Source Stack for Dash7 Alliance Protocol
https://sub-iot.github.io/Sub-IoT-Stack/
Other
150 stars 90 forks source link

ALP_ITF_ID_D7ASP interface config format doesn't seem to match spec (same with pyd7a impl) #129

Open rdaum opened 2 years ago

rdaum commented 2 years ago

D7A Specification Version 1.2 page 54, section 9.2.1 describes the Configuration of consisting of 4+ bytes with the first three bytes being QoS, TO, and TE, then followed by addressee.

What's actually emitted at https://github.com/Sub-IoT/Sub-IoT-Stack/blob/master/stack/modules/alp/alp.c#L151

Puts out 1 byte less, with just qos, dormant_timeout, and then addressee. No 'TE' byte.

Likewise the python implementation @ https://github.com/Sub-IoT/pyd7a/blob/master/d7a/sp/configuration.py#L46

Also puts out 1 byte less than the spec and is missing the 'TE' byte.

Writing an implementation that follows the specification literally, as the author of https://github.com/Stratus51/rust_dash7_alp has done, seems to lead to problems.

LOorts-Aloxy commented 2 years ago

Hi Ryan,

it seems you are right! We tried to follow the implementation as close as possible but seemed to have missed this field.

I will take a look at it and try to make a backwards compatible version for this not to break implementations relying on the current version.

In the newest, not yet released, version of the spec, we also opted to add a version field to this configuration to make sure we can solve issues like these easier.

Thank you for investigating!

Liam

LOorts-Aloxy commented 2 years ago

Ryan,

It seems this is a remnant from a previous version of the spec which did not yet include this field. Perhaps it's best to add this field under a cmake variable to ensure people can keep using the previous structure to not break backwards compatibility. In the newest (unreleased) version, byte 2 will be a version field if a flag is set in the first byte: image This is not yet released but could be taken into account for this fix as it will be included in the next spec.

Liam