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.

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

[WIP] gcoap: adapt to API change in PR 20900 #129

Closed
wants to merge 3 commits into from

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 14, 2024

In PR 20900 promotes access to header fields through helper functions on the C side, in the hope that this will allow similar but not quite UDP transports in addition to UDP (TCP and WebSocket) directly in nanocoap. This adapts and uses a helper to access the header instead.

[edit/chrysn: made PR a link]

In PR 20900 promotes access to header fields through helper functions on
the C side, in the hope that this will allow similar but not quite UDP
transports in addition to UDP (TCP and WebSocket) directly in nanocoap.
This adapts and uses a helper to access the header instead.
@maribu
Copy link
Member Author

maribu commented Oct 14, 2024

@chrysn What would be the idomatic way to do this change backward-compatible? The struct member hdr on the C side will no longer be present in the PR, at least when non-UDP transports are (also) used.

@chrysn
Copy link
Member

chrysn commented Oct 14, 2024

I'll check, but the riot-wrappers shouldn't expose any of the fields anyway, so they can just switch over to the accessors in a compatible way.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Looks good already -- is RIOT doing a release transition to provide coap_pkt_set_code and only then break the struct access API, or is this an instant-breaking change in RIOT?

If there is transition, I think we can just do this as-is already, at latest when coap_pkt_set_code is available.

If there is a breaking change without transition release, there'll need to be a marker based on which to infer the right behavior. (I've simplified markers recently because most of it could be done through riotbuild.h in recent times; not sure that's the case here).

src/gcoap.rs Outdated Show resolved Hide resolved
Co-authored-by: chrysn <[email protected]>
@maribu
Copy link
Member Author

maribu commented Oct 14, 2024

If there is transition, I think we can just do this as-is already, at latest when coap_pkt_set_code is available.

Sadly, the helper coap_pkt_set_code() is not yet in RIOT (a getter for the code was already present, though). So just switching to the setter will not build with RIOT until the PR in question is merged.

I could split out a PR to add the setter and backport that to the latest release, hoping on getting a release maintainer approval for a feature backport. But that would still bump the RIOT version requirements of the rust-riot-wrappers.

@chrysn
Copy link
Member

chrysn commented Oct 14, 2024

With the plan laid out in RIOT-OS/RIOT#20900 (comment), it should be fair to have this PR's change conditional: if any of the new modules are in (say, #[cfg(any(riot_module_gcoap_tcp, riot_module_gcoap_websockets))), the new version is used; otherwise, the old is used. If that's creating too much churn, it might be an option to create a pseudomodule gcoap_nonudp that the new ones depend on, but I don't anticipate that there'll be an inrush of transports right away.

We don't have a dependency on the cfg-if crate in the wrappers, so the old implementation would need to be kept in sync with a #[cfg(not(any(riot_module_gcoap_tcp, …)))] statement; I think that's fine, but if you think it is ugly, cfg-if could be added. (But all this will be out first thing after the next RIOT release anyway).

@chrysn
Copy link
Member

chrysn commented Oct 17, 2024

The proposed path did not work out: Neither c2rust nor bindgen support the unnamed-union trick. (In hindsight, that could have been clear: There are no unnamed unions in Rust).

The next best thing is probably introducing a marker that decides based on the presence of the new accessor whether or not to do the new thing (as Rust's rust-lang/rust#64797 is not there yet).

As that kind of markers is not around, I'm taking the liberty to add that.

@maribu
Copy link
Member Author

maribu commented Oct 17, 2024

To be fair: The unnamed union trick is also implementation defined behaviour in C. The assumption that two differently typed pointers to the data address space with the same memory as target hare bitwise identical is not backed by the C spec.

So this trick is exactly the kind of bad black magic that made a lot of people switch to rust.

@maribu maribu closed this Oct 17, 2024
@maribu maribu deleted the gcoap/compatibility-with-pr-20900 branch October 17, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants