Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

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

Open
rdaum opened this issue May 26, 2022 · 2 comments

Comments

@rdaum
Copy link
Contributor

rdaum commented May 26, 2022

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.

@rdaum rdaum changed the title ALP_ITF_ID_D7ASP interface config format doesn't seem to match spec (same with p7d7a impl) ALP_ITF_ID_D7ASP interface config format doesn't seem to match spec (same with pd7a impl) May 26, 2022
@rdaum rdaum changed the title ALP_ITF_ID_D7ASP interface config format doesn't seem to match spec (same with pd7a impl) ALP_ITF_ID_D7ASP interface config format doesn't seem to match spec (same with pyd7a impl) May 26, 2022
@LOorts-Aloxy
Copy link
Member

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
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants