Skip to content

saul: Fix type confusion#46

Closed
maribu wants to merge 1 commit intoRIOT-OS:mainfrom
maribu:fix-phydat
Closed

saul: Fix type confusion#46
maribu wants to merge 1 commit intoRIOT-OS:mainfrom
maribu:fix-phydat

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Feb 21, 2023

See RIOT-OS/RIOT#19292 for details. This brings the unit names back in line with the C variants.

@chrysn: What is the correct way to update on API changes in lock-step?

See RIOT-OS/RIOT#19292 for details. This brings
the unit names back in line with the C variants.
@maribu maribu requested a review from chrysn February 21, 2023 13:16
@chrysn
Copy link
Copy Markdown
Member

chrysn commented Feb 22, 2023

The correct way to update the API in lock-step is to retain our G unit (can't remove from the Rust API without a breaking change, but no need to because the deprecations work well), introduce a marker in riot-sys that forwards the information whether this particular change has landed in the underlying RIOT, and then use the marker to decide whether the conversion to RIOT uses the old or the new names.

How convoluted this is (precisely: That this spans across two crates) indicate I should try to make the workflow smoother; defining the markers where they are used (as opposed to where the source code is processed) should help simplify this.

I'll have a brief look into whether that simplification can be done easily already, or whether that's better done later.

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Feb 22, 2023

It seems to be straightforward enough to do the simplification now; RIOT-OS/rust-riot-sys#21 is pending its build tests. With that, the marker check can be introduced in riot-wrappers; still preparing a PR that'll set the style and add the 10-ish lines of infrastructure.

Then, riot-wrappers' build.rs should check for the presence of UNIT_G_FORCE, tell rustc to set the config "marker_unit_g_disambiguation", and then the aliases can be configured appropriately.

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Feb 23, 2023

Do I understand the status of #19292 right that this PR is not needed any more to make things work there (because there are now enum aliases instead of preprocessor redefines)?

AIU we should still do something here (basically, follow there renamings), but now it's "only" about evolving the Rust API.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 24, 2023

Yes. I intent do migrate to SenML units for phydat anyway. Especially for when dumping to JSON, the SenML unit have a clearly defined string identifier. Whereas the current unit strings are clearly targeting human audience, rather then being optimized for being machine readable.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 24, 2023

Closing in favor of #50

@maribu maribu closed this Feb 24, 2023
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