change case defaults

[burrbull]

Closes #803

cc @jonathanpallant, @Emilgardis

[Emilgardis]

I think it would be worth with this to add #[doc(alias = "<peripheral name in svd>")]

[burrbull]

Looks like starting _/digit sometimes incompatible with PascalCase.

[burrbull]

/ci diff pr

[burrbull]

I don't have ideas what to do with Freescale.

[Emilgardis]

ugh, why does the /ci triggers fail here, I have no issues with https://github.com/cross-rs/cross/blob/main/.github/workflows/try.yml ...

/ci diff pr

[github-actions[bot]]

Diff for comment

• pr

[Emilgardis]

also, sometimes the github action log messes up whitespace with colors

but, diff looks good, except

     impl RegisterBlock {
         ///0x04..0x14 - SRAM/NOR-Flash chip-select timing register %s
         #[inline(always)]
-        pub const fn btr(&self, n: usize) -> &BTR {
+        pub const fn Btr(&self, n: usize) -> &Btr {
             #[allow(clippy::no_effect)] [(); 4][n];
             unsafe { &*(self as *const Self).cast::<u8>().add(4).add(8 * n).cast() }
         }

should be btr, not Btr

[burrbull]

ugh, why does the /ci triggers fail here, I have no issues with

Are you sure someone else except you can run this?

[Emilgardis]

There's nothing in it that would prohibit other members to run it, the only check is that the user making the comment is a member, which we both are

[burrbull]

/ci diff pr

[Emilgardis]

    {
      "id": "IC_kwDOBDHJwM5yw0Sf",
      "author": {
        "login": "burrbull"
      },
      "authorAssociation": "MEMBER",

I don't understand...

/ci diff pr

[github-actions[bot]]

Diff for comment

• pr

[burrbull]

@Emilgardis run ci yet one time, please.

[Emilgardis]

can you try it, this time put some text before

[Emilgardis]

/ci diff pr

[github-actions[bot]]

Diff for comment

• pr

[Emilgardis]

/ci diff --pager "git diff --no-index" pr

[github-actions[bot]]

Diff for comment

• --pager "git diff --no-index" pr

[Emilgardis]

original behaviour would be

--ident-format field_reader::c:_R 
--ident-format field_writer::c:_W 
--ident-format enum_name::c:_A 
--ident-format enum_write_name::c:_AW 
--ident-format enum_value::c: 
--ident-format interrupt::c: 
--ident-format cluster::c: 
--ident-format cluster_accessor::c: 
--ident-format cluster_mod::s:
--ident-format register::c: 
--ident-format register_spec::c:_SPEC 
--ident-format register_accessor::c: 
--ident-format register_mod::s:
--ident-format peripheral::c:
--ident-format peripheral_singleton::c:
--ident-format peripheral_mod::s:
--ident-format peripheral_feature::c:

[burrbull]

Seems like that's all

[burrbull]

Lets try

/ci diff pr

[Emilgardis]

I don't understand why you're not able to trigger it. Might just have to switch to a check in the binary instead of relying on githubs actions environment doing things correctly...

/ci diff pr

/ci diff --current "@pr --ident-format field_reader::c:_R --ident-format field_writer::c:_W --ident-format enum_name::c:_A --ident-format enum_write_name::c:_AW --ident-format enum_value::c: --ident-format interrupt::c: --ident-format cluster::c: --ident-format cluster_accessor::c: --ident-format cluster_mod::s: --ident-format register::c: --ident-format register_spec::c:_SPEC --ident-format register_accessor::c: --ident-format register_mod::s: --ident-format peripheral::c: --ident-format peripheral_singleton::c: --ident-format peripheral_mod::s: --ident-format peripheral_feature::c:"

[github-actions[bot]]

Diff for comment

• pr

[Emilgardis]

/ci diff pr

/ci diff --current "@pr --ident-format field_reader::c:_R --ident-format field_writer::c:_W --ident-format enum_name::c:_A --ident-format enum_write_name::c:_AW --ident-format enum_value::c: --ident-format interrupt::c: --ident-format cluster::c: --ident-format cluster_accessor::c: --ident-format cluster_mod::s: --ident-format register::c: --ident-format register_spec::c:_SPEC --ident-format register_accessor::c: --ident-format register_mod::s: --ident-format peripheral::c: --ident-format peripheral_singleton::c: --ident-format peripheral_mod::s: --ident-format peripheral_feature::c:"

[github-actions[bot]]

Diff for comment

• pr

[Emilgardis]

/ci diff -c stm32f103 --current "@pr --ident-format field_reader::c:_R --ident-format field_writer::c:_W --ident-format enum_name::c:_A --ident-format enum_write_name::c:_AW --ident-format enum_value::c: --ident-format interrupt::c: --ident-format cluster::c: --ident-format cluster_accessor::c: --ident-format cluster_mod::s: --ident-format register::c: --ident-format register_spec::c:_SPEC --ident-format register_accessor::c: --ident-format register_mod::s: --ident-format peripheral::c: --ident-format peripheral_singleton::c: --ident-format peripheral_mod::s: --ident-format peripheral_feature::c:"

[github-actions[bot]]

Diff for comment

[Emilgardis]

oops, actual should be

--ident-format field_reader::c:_R                                                                                                                          
--ident-format field_writer::c:_W 
--ident-format enum_name::c:_A 
--ident-format enum_write_name::c:_AW 
--ident-format enum_value::c: 
--ident-format interrupt::c: 
--ident-format cluster::c: 
--ident-format cluster_accessor::s: 
--ident-format cluster_mod::s:
--ident-format register::c: 
--ident-format register_spec::c:_SPEC 
--ident-format register_accessor::s: 
--ident-format register_mod::s:
--ident-format peripheral::c:
--ident-format peripheral_singleton::c:
--ident-format peripheral_mod::s:
--ident-format peripheral_feature::c:

/ci diff -c stm32f103 --current "@pr --ident-format field_reader::c:_R --ident-format field_writer::c:_W --ident-format enum_name::c:_A --ident-format enum_write_name::c:_AW --ident-format enum_value::c: --ident-format interrupt::c: --ident-format cluster::c: --ident-format cluster_accessor::s: --ident-format cluster_mod::s: --ident-format register::c: --ident-format register_spec::c:_SPEC --ident-format register_accessor::s: --ident-format register_mod::s: --ident-format peripheral::c: --ident-format peripheral_singleton::c: --ident-format peripheral_mod::s: --ident-format peripheral_feature::c:"

[github-actions[bot]]

Diff for comment

• pr

[burrbull]

cc @Emilgardis

[BrokenR3C0RD]

Tested this on a PAC I'm working on for the LPC11U1x/2x/3x MCUs, and it leads to some bad names for pins. In the SVD and reference manual, they all follow the naming scheme of PIO{port}_{pin}, where port is 0,1, and pin is 0..=31. However, the defaults used in this branch result in Pio{port}{pin}, resulting in pin types being named things like Pio19, Pio110, etc. Additionally, some of the enumeratedValues names get treated poorly too; one example is BETWEEN_1_20MHZ, which turns into Between120Mhz, and BETWEEN_15_20MHZ, which turns into Between1520Mhz.

Personal opinion, but I believe defaults should keep underscores between numbers; Between15_20Mhz isn't the prettiest, but would be more sensible than the current defaults being used in this branch.

[burrbull]

Tested this on a PAC I'm working on for the LPC11U1x/2x/3x MCUs, and it leads to some bad names for pins. In the SVD and reference manual, they all follow the naming scheme of PIO{port}_{pin}, where port is 0,1, and pin is 0..=31. However, the defaults used in this branch result in Pio{port}{pin}, resulting in pin types being named things like Pio19, Pio110, etc. Additionally, some of the enumeratedValues names get treated poorly too; one example is BETWEEN_1_20MHZ, which turns into Between120Mhz, and BETWEEN_15_20MHZ, which turns into Between1520Mhz.

Personal opinion, but I believe defaults should keep underscores between numbers; Between15_20Mhz isn't the prettiest, but would be more sensible than the current defaults being used in this branch.

It looks like we need custom logic for to_pascal_case instead of built-in. I've tried to make some workarounds but those fix only individual cases.

[burrbull]

@BrokenR3C0RD try last changes

[BrokenR3C0RD]

@burrbull Works as expected, PIO0_11 becomes Pio0_11 and BETWEEN_15_20_MHZ becomes Between15_20Mhz. However, I think there might be an issue with 324b12c (which stops the panic in #811). SVD

Generates the following for PIO1:

// ...
impl RegisterBlock {
    #[doc = "0x60..0xe0 - Generic I/O configuration for PIO pins, port 1."]
    #[inline(always)]
    pub const fn pio1(&self, n: usize) -> &Pio10 {
        #[allow(clippy::no_effect)]
        [(); 32][n];
        unsafe { &*(self as *const Self).cast::<u8>().add(96).add(4 * n).cast() }
    }
    #[doc = "Iterator for array of:"]
    #[doc = "0x60..0xe0 - Generic I/O configuration for PIO pins, port 1."]
    #[inline(always)]
    pub fn pio1_iter(&self) -> impl Iterator<Item = &Pio10> {
        (0..32)
            .map(move |n| unsafe { &*(self as *const Self).cast::<u8>().add(96).add(4 * n).cast() })
    }
}

#[doc = "PIO0 (rw) register accessor: Generic I/O configuration for PIO, port 0.\n\nYou can [`read`](crate::generic::Reg::read) this register and get [`pio0::R`].  You can [`write_with_zero`](crate::generic::Reg::write_with_zero) this register using [`pio0::W`]. You can also [`modify`](crate::generic::Reg::modify) this register. See [API](https://docs.rs/svd2rust/#read--modify--write-api).\n\nFor information about available fields see [`mod@pio0`]
module"]
#[doc(alias = "PIO0")]
pub type Pio0 = crate::Reg<pio0::Pio0Spec>;
#[doc = "Generic I/O configuration for PIO, port 0."]
pub mod pio0;
pub use pio0 as pio1;
pub use Pio0 as Pio1;

Note that Pio10 is referenced instead of Pio1.

[burrbull]

Note that Pio10 is referenced instead of Pio1.

I think this is due to https://github.com/rust-embedded/svd2rust/blob/a23a780b091b238299ad91abb267fb9a2cbadef2/src/generate/peripheral.rs#L1192-L1199. But I don't remember when and why it was added. Maybe this is not needed more.

[Emilgardis]

git blame reveals it was https://github.com/rust-embedded/svd2rust/pull/662

[Emilgardis]

/ci diff pr

[github-actions[bot]]

Diff for comment

• pr

[BrokenR3C0RD]

a085e4e now generates the correct pio1/pio1_iter functions for me, so #811 can be closed when/if this is merged. Thanks @burrbull!

[burrbull]

this is not solved, an invalid name, e.g cluster_accesor doesn't warn

Fixed.

@Emilgardis can we discuss themes in separate PR?

[Emilgardis]

@Emilgardis can we discuss themes in separate PR?

Sounds good!

[Emilgardis]

Should we discuss this broader or are we good with going forward with this breaking change? I kinda want one more team member to ack

[burrbull]

Should we discuss this broader or are we good with going forward with this breaking change? I kinda want one more team member to ack

I can split of default formats changing (maybe with themes) in separate PR and we discuss them on next meeting.

[Emilgardis]

Lets keep the default changes to this pr but split out the ident-format improvements to a new pr which we can then just merge

[burrbull]

Lets keep the default changes to this pr but split out the ident-format improvements to a new pr which we can then just merge

812

[burrbull]

I'm not sure myself if that is the best way to go about it, maybe we could have "themes" where there's a default theme (the new behaviour), and another theme that looks like how it used to look like. You can still set individual formats if wanted. so maybe

ident_formats_theme = "legacy"

Done. Not sure about new theme name.

[Emilgardis]

Done. Not sure about new theme name.

We could also not have a new theme, e.g not expose it

[Emilgardis]

maybe default?

With this variant though (legacy and default/new), we get into a issue of versioning, where if we change the theme we'd probably have to introduce a new theme for the previous version. I think only having legacy makes sense for now. Maybe there's an argument for having ranged theme names, such that saying theme = "0.35.1" means you get the appropriate theme for that version, but sounds like hell to maintain and keep track of

[thejpster]

2021 and 2024? (Or whatever year makes sense)

[burrbull]

@Emilgardis Can you fix svd2rust-regress "unused" warnings?