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

scb: derive serde, Hash, PartialOrd for VectActive behind gates #363

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

tmplt
Copy link
Contributor

@tmplt tmplt commented Nov 17, 2021

Exposes two new feature gates for VectActive serde::{Serialize,
Deserialize} (via "serde") and Hash, PartialOrd (via "std-map") for use
on host-side ITM tracing programs. While the struct itself is not
received directly over ITM, its use greatly simplifies the
implementation by allowing VectActive as keys in map collections and
file/socket {,de}serialization to forward the structure elsewhere. These
features are not enabled by default.

Before this patch, serde functionality could be realized via [0], but
this does not propagate down a dependency chain (i.e. if realized for
crate B, which crate A depends on, serde functionality is not exposed in
crate A unless VectActive is wrapped in a type from crate B). I am not
aware of any method to realize PartialOrd, Hash derivation for a
downstream crate.

[0] https://serde.rs/remote-derive.html

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thalesfragoso (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Nov 17, 2021
thejpster
thejpster previously approved these changes Nov 17, 2021
Copy link
Contributor

@thejpster thejpster left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

The format test is failing. Please could you run cargo fmt and push the changes.

@thejpster
Copy link
Contributor

Maybe we should expand the CI coverage to compile the crate into a non-Cortex-M host application, to check people can use the types we define for host-side purposes.

@tmplt
Copy link
Contributor Author

tmplt commented Nov 17, 2021

I'll take a look at the CI and amend fmt fixes later this week.

Cargo.toml Outdated Show resolved Hide resolved
Exposes two new feature gates for VectActive serde::{Serialize,
Deserialize} (via "serde") and Hash, PartialOrd (via "std-map") for use
on host-side ITM tracing programs. While the struct itself is not
received directly over ITM, its use greatly simplifies the
implementation by allowing VectActive as keys in map collections and
file/socket {,de}serialization to forward the structure elsewhere. These
features are not enabled by default.

Before this patch, serde functionality could be realized via [0], but
this does not propagate down a dependency chain (i.e. if realized for
crate B, which crate A depends on, serde functionality is not exposed in
crate A unless VectActive is wrapped in a type from crate B). I am not
aware of any method to realize PartialOrd, Hash derivation for a
downstream crate.

[0] https://serde.rs/remote-derive.html
@tmplt
Copy link
Contributor Author

tmplt commented Nov 19, 2021

Latest force-push fixes formatting and adds a check_host_side xtask that check serde and PartialOrd functionality.

Copy link
Member

@thalesfragoso thalesfragoso left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 25, 2021

Build succeeded:

@bors bors bot merged commit a78c497 into rust-embedded:master Nov 25, 2021
tmplt added a commit to rtic-scope/itm that referenced this pull request Nov 27, 2021
@adamgreig adamgreig mentioned this pull request Dec 31, 2021
bors bot added a commit that referenced this pull request Jan 2, 2022
375: Prepare v0.7.4 r=thejpster a=adamgreig

I've created a new branch, `v0.7.x`, which is currently at the latest non-breaking commit (so includes #346 #349 #347 #351 #339 #352 #348 #363 #362 #361 but does not include #342), to track the 0.7 series since master now contains breaking changes for v0.8.

This PR (which targets the new branch) cherry-picks #372 #369 #374 and bumps the version to v0.7.4 (and updates CHANGELOG) ready for a new v0.7.4 release. Once complete I'll also backport the changelog entries and bump the version in master to 0.7.4.

I think this is everything that should be in 0.7 -- the only excluded PRs from master are #342 and #367 I believe, and I don't think we have any open PRs targeting 0.7 either.

Any other thoughts on items for inclusion in 0.7.4 (or other changelog entries I missed)?

Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Co-authored-by: Adam Greig <adam@adamgreig.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants