Fix gd32e1

[kaizensparc]

There will be a lot a small (and some not so small) fixes on the GD32E1 PAC crate during the following days. I'm trying to put them all in this PR before I'll submit it to review

[github-actions]

Memory map comparison

[github-actions]

Memory map comparison

[kaizensparc]

Hi! Happy new year :) This PR should now be ready for a review. I made changes to the timers that should still work in a similar way, let me know if something is not correct :)

[qwandor]

What's the advantage of this over giving a range like we had before? It's marginally longer, and I find the range easier to read.

[kaizensparc]

From what I read in svdtools, ranges are not supported. This means it was trying to match "TIMER13", "TIMER1-" and "TIMER16" before :)

[qwandor]

Are you sure? It seemed to be working fine before, and the stm32-rs repository is using it, e.g. in https://github.com/stm32-rs/stm32-rs/blob/master/peripherals/tim/tim_basic.yaml.

[kaizensparc]

If it uses this svdtools I don't see any way to use ranges. The parsing code doesn't seem to handle it (https://github.com/stm32-rs/svdtools/blob/074c31f67938ae21efeb2c74fe01260d7e4f55ff/src/patch/mod.rs#L486) and the documentation has a very small set of tokens (https://github.com/stm32-rs/svdtools#name-matching).
It was not working when I was using it, e.g. TIM[1-58] was matching TIM1, TIM5 and TIM8 (so was not reporting an error not finding TIM- because it only needs one match to work), but not TIM2

[qwandor]

We're using the Python version of svdtools still, so maybe it has more functionality? It does look like there is a bug here though, so I'll have a proper look.

[qwandor]

I just tried building locally, and I get exactly the same output in the svd/*.svd.patched files with or without your change to stop using ranges, so I'm pretty sure the svd patch is handling them properly as expected. Removing my _derivedFrom fixes does change the output, so we don't want to change that.

I'm not sure why it's not working for you. How are you trying to build it, just using make? What version of svdtools do you have installed, and did you install from pip or somewhere else?

[kaizensparc]

Oooh, I was using the rust version. The python version uses braceexpand/fnmatch which handles these kind of things. I'll fix that on my machine and try to rollback the other ones I had unrolled in previous versions too then :)

[qwandor]

Doesn't removing all these _derivedFrom fixes result in the fields having different types? That's going to break the HAL crate, and mean a lot more code in the PAC.

[kaizensparc]

The codegen gave the same types for the identical timers (timer1/2/3/4 for instance), but the type itself is different between different timers indeed :/

[qwandor]

That's going to be a problem then, please keep the _derivedFrom fixes. My HAL code depends on them being the same type.

[kaizensparc]

I get issues when I try to use the derived peripherals:

TIMER0:
  CTL0:
    CEN:
      Disabled: [0, "Counter disabled"]
      Enabled: [1, "Counter enabled"]

"TIMER[1-9],TIMER1[0-9]":
  CTL0:
    CEN:
      _derivedFrom: "TIMER0.CTL0.CEN.CEN"

If I try to use cen here for instance I get:

error[E0599]: no method named `disabled` found for struct `BitWriterRaw<'_, u32, gd32e1::gd32e103::timer1::ctl0::CTL0_SPEC, CEN_A, BitM, 0>` in the current scope
   --> src/timer.rs:361:55
    |
361 |                 self.timer.ctl0.modify(|_, w| w.cen().disabled());
    |                                                       ^^^^^^^^ method not found in `BitWriterRaw<'_, u32, gd32e1::gd32e103::timer1::ctl0::CTL0_SPEC, CEN_A, BitM, 0>`

The read is working though (let is_counter_enabled = self.timer.ctl0.read().cen().is_enabled(); compiles)

[kaizensparc]

I tried compiling the gd32f1x0-hal library on this repo's main branch and I got the same error 🤔
Same error using https://github.com/gd32-rust/gd32-rs-nightlies too

[qwandor]

Hmm, I guess I'm going to need to bisect then. I probably won't have time for a few days though. If you figure out what's going on in the meantime let me know.

If it would be helpful I'm happy to merge everything except for the timer changes for now.

[kaizensparc]

I think this is a change that happened with the new svd2rust. During code generation the enum type and reader type are just linked to the original one, but the writer is put as a new type. The issue is either that the writer is defined as a new type or that the writer doesn't have any implementation (it should have the methods to set the register values). I'm trying to look in svd2rust's code to understand how this happened.
I don't need a merge right now, we can try and fix this and then merge :)

[kaizensparc]

Found it, it appears to be a bug in svd2rust. This crate in v0.6.0 was using svd2rust 0.24.0 (which have been yanked from the registry), and the bug appeared in svd2rust 0.25.0.
I submitted an issue on their repo: rust-embedded/svd2rust#705

[github-actions]

Memory map comparison

[qwandor]

I've merged everything except for the timer changes in #41. I guess we need to wait for the fix to rust-embedded/svd2rust#705 before releasing anything.

[kaizensparc]

Thanks, I'm waiting for their answer, in the meantime I'll work with the duplicated version in the timers to implement the hal and will change back when the fix is implemented.

[kaizensparc]

Fixed by #51