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

refactor(embassy)!: extract arch modules as crates #392

Conversation

ROMemories
Copy link
Collaborator

@ROMemories ROMemories commented Aug 29, 2024

Description

This PR extracts the arch modules (the manufacturer-specific modules previously found in riot-rs-embassy) as separate, manufacturer-specific crates. A major benefit of this is allowing to generate user-visible documentation for these crates, which was not possible before because (a) only one arch module could be enabled at a time and (b) it was not possible to enable any of them for documentation, because of them being manufacturer-specific.

Extracting these modules as separate crates has required to:

  • Introduce a new crate (whose temporary name is riot-rs-shared-types) to break cyclic dependencies between crates/types: this new crate is depended upon by every manufacturer-specific crate.
  • Revisit some types in the new riot-rs-shared-types crate to accommodate manufacturer-specific variants in enums, by introducing simple generics on them.
  • Work around the orphan rules in some internal cases.
  • Introduce the relevant Cargo features for individual manufacturer-specific crates. Only features that are actually useful for the crate are introduced. This introduces a slight increase in duplicated features in the tree, but has the benefit of documenting what features are actually provided by each manufacturer-specific crate.

Issues/PRs references

Depends on #391
Depends on #394

Open Questions

Testing

The following have been successfully tested in hardware (on dfaca71):

  • example/gpio on nRF52840-DK, nRF5340-DK, ESP32-C6, RP2040, ST-NUCLEO-WB55RG
  • example/embassy-http-server
    • on ESP32-C6, RPi Pico W with Wi-Fi
    • on nRF52840-DK, ST-NUCLEO-WB55RG on Ethernet over USB

TODO

  • Clean up the circumventions around the orphan rule limitations

Future work

  • Generate documentation for these new manufacturer-specific crates

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@ROMemories ROMemories added the arch Architecture support label Aug 29, 2024
@ROMemories ROMemories force-pushed the refactor/extract-arch-modules-as-crates branch from a412d4b to 1b1405f Compare August 30, 2024 07:40
@ROMemories ROMemories force-pushed the refactor/extract-arch-modules-as-crates branch 8 times, most recently from 7619fc9 to dfcd104 Compare September 2, 2024 14:45
@ROMemories ROMemories marked this pull request as ready for review September 2, 2024 15:10
@kaspar030
Copy link
Collaborator

Please squash & rebase. Looks fine already, I'll give it a final review by tomorrow.

@ROMemories ROMemories force-pushed the refactor/extract-arch-modules-as-crates branch 4 times, most recently from dfaca71 to 552dd4d Compare September 11, 2024 14:37
Copy link
Collaborator

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

I'd say this is almost good to go.
Please do the rename riot-rs-shared-types -> riot-rs-embassy-common, and move the now duplicated wifi config there.

@kaspar030 kaspar030 mentioned this pull request Sep 12, 2024
4 tasks
@ROMemories ROMemories force-pushed the refactor/extract-arch-modules-as-crates branch from 2139964 to df1e30e Compare September 12, 2024 12:45
@ROMemories ROMemories force-pushed the refactor/extract-arch-modules-as-crates branch from df1e30e to 53bb828 Compare September 12, 2024 12:51
Copy link
Collaborator

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

LGTM.

@kaspar030 kaspar030 added this pull request to the merge queue Sep 13, 2024
Merged via the queue into future-proof-iot:main with commit ca902cd Sep 13, 2024
26 checks passed
ROMemories added a commit to ROMemories/RIOT-rs that referenced this pull request Sep 18, 2024
@ROMemories ROMemories mentioned this pull request Sep 27, 2024
4 tasks
elenaf9 pushed a commit to elenaf9/RIOT-rs that referenced this pull request Sep 27, 2024
elenaf9 pushed a commit to elenaf9/RIOT-rs that referenced this pull request Sep 27, 2024
elenaf9 pushed a commit to elenaf9/RIOT-rs that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch Architecture support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants