Update PAC's to svd2rust 0.30.2 #683

Tremoneck · 2023-05-14T09:13:47Z

Summary

Update the PAC to use svd2rust 0.28.0 and form 0.10.0, while fixing up all the renamed registers.
svd2rust renamed severall registers from *_A to *SELECT_A, these were fixed up. This is a breaking change for dependent crates accessing registers, structs or enums directly.

Two patches were neccessary to make the generated code correct.

For the SAM_5 series the EVSYS generated a struct and a register both called CHANNEL. The register was renamed to CHANNELCTRL to remove the name conflict
For some of the SAMD2 series a type is named differently between usage and definition. This was fixed manually and the corresponding issue is Disjoint array handling issue with Microchip SAM L21 rust-embedded/svd2rust#713

This pull request isn't finished as is, but requires some feedback:
For the SAM_5 should the register be renamed or the struct representing a channel?
Should we wait for a fix to land for rust-embedded/svd2rust#713, go back to 0.27.0 or go with this manual patch?
The new generated code relies on a feature on critical-section. Should this be default and always available, or should it, like it is in this PR be a optional feature which can be enabled over the hal?

Fixes #648
Obsoletes #650

Checklist

CHANGELOG.md for the BSP or HAL updated
All new or modified code is well documented, especially public items
No new warnings or clippy suggestions have been introduced (see CI or check locally)

If Adding a new cargo `feature` to the HAL

Feature is added to the test matrix for applicable boards / PACs in crates.json

See rust-embedded/svd2rust#713

The Parts changed where adding SELECT to a few Registers ending with _A, as they were renamed frm *_A to *SELECT_A

jbeaurivage · 2023-05-14T13:03:52Z

@Tremoneck, thank you for this PR! I've had something similar but half finished for a while. Nice to see someone else step up to take it on.

To answer your questions:

For the SAM_5 should the register be renamed or the struct representing a channel?

I think logically the register should be renamed. Looks like an oversight on Atmel's part, as most registers are suffixed with *CTRL. That being said, I don't particularly like that the PACs now diverge from the datasheet.

Should we wait for a fix to land for rust-embedded/svd2rust#713, go back to 0.27.0 or go with this manual patch?

That depends. Do you expect rust-embedded/svd2rust#713 to end up generating the same code as your manual patch? Ie, will we have breaking changes when a fix lands and we re-generate the PACs in the future?,

Tremoneck · 2023-05-14T17:26:37Z

I don't like the divergent from the Datasheet either. In the Datasheet (p. 792) the register is titled Channel n Control but the name is given as the value in the SVD of CHANNEL. The alternative is calling the struct something along the lines of CHANNEL_REGISTERS.

I expect the register to end up generating to the value I fixed them to. It's the same type name as the 0.27 would generate. So even if a fix changes the typename, we would still have the same breaking change.

jbeaurivage · 2023-05-14T18:20:27Z

I don't like the divergent from the Datasheet either. In the Datasheet (p. 792) the register is titled Channel n Control but the name is given as the value in the SVD of CHANNEL. The alternative is calling the struct something along the lines of CHANNEL_REGISTERS.

What about calling the struct CHANNELS?

In any case, I'm on board with the manual patch while awaiting the svd2rust fix, especially if any migration steps are properly documented. Let me know if you have any more questions. Obviously new PAC/BSP/HAL releases will be necessary when this PR is merged.

This change keeps the register names in sync with the datasheet.

Tremoneck · 2023-05-17T20:58:30Z

Where should the breaking changes be documented? Should that be in the changelog on how to migrate to the next version.

I've added a new feature to the hal, which is only needed if you want to use the take for Peripherals. Where should this be documented? I really would like to add an example for when you need it and what else you need so you don't get compiler errors.

When everything is ready, should I rebase and squash the commits and add a longer explanation into the commit message with the required things to do to migrate.

jbeaurivage · 2023-05-26T18:39:08Z

Where should the breaking changes be documented? Should that be in the changelog on how to migrate to the next version.

The changelog, and maybe a short note in the readme as well?

I've added a new feature to the hal, which is only needed if you want to use the take for Peripherals. Where should this be documented? I really would like to add an example for when you need it and what else you need so you don't get compiler errors.

What exactly does that feature entail? Is is something that's mandated by the new svd2rust version?

When everything is ready, should I rebase and squash the commits and add a longer explanation into the commit message with the required things to do to migrate.

Yep, that sounds good!

Tremoneck · 2023-05-27T06:24:36Z

pac/atsamd11c/src/lib.rs

@@ -914,18 +913,22 @@ pub struct Peripherals {
    pub WDT: WDT,
 }
 impl Peripherals {
-    #[doc = r"Returns all the peripherals *once*"]
+    #[doc = r" Returns all the peripherals *once*."]
+    #[cfg(feature = "critical-section")]


The optional dependency is forced by this new addition to the generated pac. To access the peripherals one has to call take. But only one thread may get the struct. To avoid a datarace synchronization is required. This used to be achieved with a cortex specific call to disable interrupts, but was replaced with a critical section. One can still use steal if one wants to avoid the dependency's, but has to use unsafe code.

Ah fair enough. Though I don't really see a case where someone wouldn't want to use the critical section feature - I would suggest enabling it in the HAL for all PACs. That way users and libraries can use thecritical-section lib without having to worry about cargo features. After all, it's not like there should be another critical section provider in the dependency tree anyways - what do you think?

jbeaurivage · 2023-06-10T19:12:02Z

@Tremoneck, svd2rust 0.29.0 is out. I don't know if you want to adapt to the new version, or stick to 0.28.0?

Tremoneck · 2023-06-10T20:59:03Z

I've been thinking longg about the critical-section part. I believe you are right with providing always. However, we will need documentation in that case that you will require another crate, that provides the bindings, as otherwise you will get strange linker errors.

Might as well switch the script to 0.29, if I'm already updating it.

jbeaurivage · 2023-06-11T01:47:36Z

I've been thinking longg about the critical-section part. I believe you are right with providing always. However, we will need documentation in that case that you will require another crate, that provides the bindings, as otherwise you will get strange linker errors.

What if the PACs depended directly on cortex-m with the critical-section-single-core feature? That would directly take care of providing the critical-section bindings if I'm not mistaken.

jbeaurivage · 2023-10-15T21:30:43Z

@Tremoneck, have you had the time to think about my last comment? I'm going to have a bit more bandwidth in the following weeks, I could assist in getting this ready for merge.

Tremoneck · 2023-10-23T18:17:23Z

The critical section crate strongly encourages gating the implementation of critical sections behind a feature flag, so that it can be disabled, as linking two creates implementing it leads to two definitions and then linker errors. It would also make this crate incompatible with any crate that wants to implement it's own definition like a RTOS or an async runtime.

For reference the STM32 crates https://github.com/stm32-rs/stm32-rs-nightlies/blob/3767c5e39a9c15e321c0230e53742f14b882be06/stm32f4/src/stm32f405/mod.rs#L4375 implement it as an optional feature as well. It is also designed in a way, where a user doesn't have to depent on an Implementor of a critical-section, as long as a critical section provider is linked in, it works.

jbeaurivage · 2023-10-24T14:56:38Z

@Tremoneck that's fair. I suggest we follow stm32-rs' lead and expose a critical-section feature flag, and make it a default feature. That way users that are having linker collisions can easily disable it. Also, I don't know how you feel about upgrading to svd2rust 0.30? Hopefully it doesn't introduct other breaking changes.

jbeaurivage · 2023-10-24T21:17:45Z

@Tremoneck, take a look at #1. If you agree and merge it, we could land this promptly.

jbeaurivage · 2023-10-24T23:01:34Z

I thought some more about the critical-section issue. I think we're getting two things mixed up here:

First, the critical-section crate recommends the feature-gating the critical section implementation. That is, if the PACs were providing their own idea of how a critical section should be implemented. In our case, an implementation is provided for us via the critical-section-single-core of the cortex-m crate. The PACs don't provide their own implementation, so we don't have to worry about it. The end user just has to include cortex-m as a dependency and all crates that depend on critical-section will now have access to that implementation. What would cause problems is if another crate also provided their own implementation, but no conflicts will ever happen with our PACs and HAL.
svd2rust also feature-gates their use of Peripherals::take() only because some platforms don't have a critical-section implementation defined already (see this and this). Feature-gating its use allows to mitigate that for those platforms specifically.

In our case, since cortex-m defines a critical-section impl, and our own crates don't, it's absolutely safe to use it unconditionally in the PACs' dependencies. My PR now reflects that.

Merge jbeaurivage updates into Branch

Tremoneck · 2023-10-25T20:04:23Z

Yeah, thats a good point. Maybe I miss understood what the critical-section crate meant. Anyway, should there ever arise a need to remove this direct dependency on the cortex-m-rt crate it should still be possible to break it out as a default feature flag without breaking SemVer I think. I've merged your commits into my feature branch.

jbeaurivage · 2023-10-26T01:41:38Z

Yeah sorry about that, I messed up the branch on top of which my PR was based. Thanks for catching it.
If you merge #3 to fix the builds I will land this. Thank you for following through!

Remove "critical-section" feature of PAC libs

ianrrees · 2023-11-13T21:01:23Z

I've noticed some new warnings for warning: unknown lint: `private_bounds` and private_interfaces, which seem to be caused by this update in combination with stable Rust. As a meta-question, are you guys generally using unstable Rust?

jbeaurivage · 2023-11-15T02:55:54Z

@ianrrees I usually use nightly compilers when working on async stuff (especially my HAL's async branch. However even that is going to change with AFIT around the corner. Otherwise I try to stick with stable.

It's true that svd2rust seems to regularly try using some weird lints, perhaps too aggressively. I'm not too sure how to fix that on our end though.

ianrrees · 2023-11-15T19:33:13Z

I see, thanks! Will aim to follow up with svd2rust and see if we can arrive at a solution that works with both stable and unstable.

ianrrees · 2023-11-16T02:47:23Z

Aha, svd2rust is already on it: rust-embedded/svd2rust#760

In the meantime, I'll PR an addition of #[allow(unknown_lints)] to the affected PACs.

ianrrees · 2023-11-16T23:28:35Z

In the meantime, I'll PR an addition of #[allow(unknown_lints)] to the affected PACs.

Disregard - Rust 1.74.0 has these new lints

* Update svd2rust and form version * CreatePatch file for the EVSYS channel * Regenerate PAC's with svd2rut 0.30.2 * Apply manual patch for wrong generation of svd2rust See rust-embedded/svd2rust#713 * Update Cargo.toml to the new Versions of the pacs * Update hal to the new pacs The Parts changed where adding SELECT to a few Registers ending with _A, as they were renamed frm *_A to *SELECT_A * Make the tier one examples compileable * Update the changelog * Change the rename in EVSYS to the Channel struct. This change keeps the register names in sync with the datasheet. --------- Co-authored-by: Adrian Geipert <adrian.git@geipert.eu> Co-authored-by: Justin Beaurivage <justin@wearableavionics.com>

Adrian Geipert added 11 commits May 8, 2023 20:31

Update svd2rust and form version

8723087

CreatePatch file for the EVSYS channel

c53b3d7

Regenerate PAC's with svd2rut 0.28.0

054e4fe

Apply manual patch for wrong generation of svd2rust

abfa485

See rust-embedded/svd2rust#713

Update Cargo.toml to the new Versions of the pacs

918eea5

Update hal to the new pacs

7be5e8e

The Parts changed where adding SELECT to a few Registers ending with _A, as they were renamed frm *_A to *SELECT_A

Create new optional dependency for critical sections

7f02579

Make the tier one examples compileable

9d20393

Update the changelog

d078c06

Fix formating

a5103a5

Fix wio_terminal

59df225

Tremoneck force-pushed the updatePAC branch from a78c73d to 59df225 Compare May 14, 2023 10:33

Tremoneck changed the title ~~Update pac~~ Update PAC's to svd2rust 0.28.0 May 14, 2023

Rename port config variables back to the right name

328d713

Adrian Geipert added 2 commits May 15, 2023 20:51

Add critical section to crates.json

9f1f658

Change the rename in EVSYS to the Channel struct.

91a2f19

This change keeps the register names in sync with the datasheet.

Tremoneck marked this pull request as ready for review May 17, 2023 20:53

Tremoneck commented May 27, 2023

View reviewed changes

Merge atsamd-rs/master

85feef0

jbeaurivage added 4 commits October 24, 2023 16:22

Update to svd2rust 0.30.2

b11b060

Update changelogs

3833523

Make critical-section a default feature and update changelog

74d6848

Turn on critical-section-single-core cortex-m flag in PACs

7666fce

jbeaurivage and others added 6 commits October 24, 2023 19:13

Remove feature-gating of critical-section in PACs and update changelogs

43931de

Add changelog entries for Tier 1 boards

54651e7

Merge branch 'master' into tremoneck_updatePAC

06c5dae

Remove critical-section features from crates.json

2803d11

Remove critical-section features of T1 BSPs

1be89f3

Merge pull request #2 from jbeaurivage/tremoneck_updatePAC

932f2e9

Merge jbeaurivage updates into Branch

Remove "critical-section" feature of PAC libs

2b8d954

jbeaurivage and others added 3 commits October 25, 2023 22:04

Fix wio_terminal build

3c51912

wio_terminal is not a Tier 1 board

094fd0a

Merge pull request #3 from jbeaurivage/tremoneck_updatePAC

8a199c2

Remove "critical-section" feature of PAC libs

jbeaurivage changed the title ~~Update PAC's to svd2rust 0.28.0~~ Update PAC's to svd2rust 0.30.2 Oct 26, 2023

jbeaurivage merged commit 1d1bdd5 into atsamd-rs:master Oct 26, 2023
105 checks passed

pabigot mentioned this pull request Nov 8, 2023

Remove const_err lint #650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PAC's to svd2rust 0.30.2 #683

Update PAC's to svd2rust 0.30.2 #683

Tremoneck commented May 14, 2023 •

edited

Loading

jbeaurivage commented May 14, 2023

Tremoneck commented May 14, 2023

jbeaurivage commented May 14, 2023

Tremoneck commented May 17, 2023

jbeaurivage commented May 26, 2023

Tremoneck May 27, 2023 •

edited

Loading

jbeaurivage May 27, 2023

jbeaurivage commented Jun 10, 2023

Tremoneck commented Jun 10, 2023

jbeaurivage commented Jun 11, 2023 •

edited

Loading

jbeaurivage commented Oct 15, 2023

Tremoneck commented Oct 23, 2023

jbeaurivage commented Oct 24, 2023

jbeaurivage commented Oct 24, 2023

jbeaurivage commented Oct 24, 2023

Tremoneck commented Oct 25, 2023

jbeaurivage commented Oct 26, 2023 •

edited

Loading

ianrrees commented Nov 13, 2023

jbeaurivage commented Nov 15, 2023

ianrrees commented Nov 15, 2023

ianrrees commented Nov 16, 2023

ianrrees commented Nov 16, 2023

Update PAC's to svd2rust 0.30.2 #683

Update PAC's to svd2rust 0.30.2 #683

Conversation

Tremoneck commented May 14, 2023 • edited Loading

Summary

Checklist

If Adding a new cargo feature to the HAL

jbeaurivage commented May 14, 2023

Tremoneck commented May 14, 2023

jbeaurivage commented May 14, 2023

Tremoneck commented May 17, 2023

jbeaurivage commented May 26, 2023

Tremoneck May 27, 2023 • edited Loading

Choose a reason for hiding this comment

jbeaurivage May 27, 2023

Choose a reason for hiding this comment

jbeaurivage commented Jun 10, 2023

Tremoneck commented Jun 10, 2023

jbeaurivage commented Jun 11, 2023 • edited Loading

jbeaurivage commented Oct 15, 2023

Tremoneck commented Oct 23, 2023

jbeaurivage commented Oct 24, 2023

jbeaurivage commented Oct 24, 2023

jbeaurivage commented Oct 24, 2023

Tremoneck commented Oct 25, 2023

jbeaurivage commented Oct 26, 2023 • edited Loading

ianrrees commented Nov 13, 2023

jbeaurivage commented Nov 15, 2023

ianrrees commented Nov 15, 2023

ianrrees commented Nov 16, 2023

ianrrees commented Nov 16, 2023

Tremoneck commented May 14, 2023 •

edited

Loading

If Adding a new cargo `feature` to the HAL

Tremoneck May 27, 2023 •

edited

Loading

jbeaurivage commented Jun 11, 2023 •

edited

Loading

jbeaurivage commented Oct 26, 2023 •

edited

Loading