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.

Sign up for GitHub

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

Fix gd32e1 #40

Closed
wants to merge 5 commits into from
Closed

Fix gd32e1 #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0839498
Merge ADCPSC in RCU
Dec 1, 2022
16f4380
Add quotes to APB[12]PSC field
Dec 1, 2022
720bd0c
Fix GD32E1's architecture
Dec 1, 2022
845cdb6
Add ADC peripheral to GD32E103.
Jan 3, 2023
5c4d51f
Fix matching issues in timer
Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions devices/common_patches/gd32e1xx.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@ RCU:
name: "CKOUTSEL"
PREDV0_LSB:
name: "PLLPREDV"
ADCPSC_1_0:
name: "ADCPSC"
USBFSPSC:
name: "USBFSPSC_3"
USBFSPSC_1_0:
name: "USBFSPSC"
_merge:
ADCPSC:
- ADCPSC_1_0
- ADCPSC_2
- ADCPSC_3

"SPI*":
DATA:
Expand Down
2 changes: 1 addition & 1 deletion devices/gd32e103.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ _svd: ../svd/gd32e103.svd

_include:
- common_patches/gd32e1xx.yaml
#- ../peripherals/adc/adc.yaml
- ../peripherals/adc/adc_e1.yaml
- ../peripherals/can/can.yaml
- ../peripherals/crc/crc.yaml
- ../peripherals/dac/dac_e1.yaml
Expand Down
1 change: 1 addition & 0 deletions peripherals/adc/adc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
_include:
- "adc_common.yaml"
- "eteic.yaml"
- "swrcst.yaml"
6 changes: 0 additions & 6 deletions peripherals/adc/adc_common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,6 @@ ADC*:
TSVREN:
Disabled: [0, "Channel 16 and 17 disabled"]
Enabled: [1, "Channel 16 and 17 enabled"]
SWRCST:
_read:
Started: [0, "Reset state"]
NotStarted: [1, "Starting conversion of regular channels"]
_write:
Start: [1, "Start conversion of regular channels"]
SWICST:
_read:
Started: [0, "Reset state"]
Expand Down
7 changes: 7 additions & 0 deletions peripherals/adc/adc_e1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright 2023 The gd32-rs authors.
#
# SPDX-License-Identifier: MIT OR Apache-2.0

_include:
- "adc_common.yaml"
- "eteic.yaml"
1 change: 1 addition & 0 deletions peripherals/adc/adc_f2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ _include:
- "adc_common.yaml"
- "adc_high_f2_f3.yaml"
- "eteic.yaml"
- "swrcst.yaml"
1 change: 1 addition & 0 deletions peripherals/adc/adc_f3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
_include:
- "adc_common.yaml"
- "adc_high_f2_f3.yaml"
- "swrcst.yaml"
12 changes: 12 additions & 0 deletions peripherals/adc/swrcst.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright 2023 The gd32-rs authors.
#
# SPDX-License-Identifier: MIT OR Apache-2.0

ADC*:
CTL1:
SWRCST:
_read:
Started: [0, "Reset state"]
NotStarted: [1, "Starting conversion of regular channels"]
_write:
Start: [1, "Start conversion of regular channels"]
2 changes: 1 addition & 1 deletion peripherals/rcu/rcu_common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ RCU:
Div4: [1, "CK_APB2 divided by 4"]
Div6: [2, "CK_APB2 divided by 6"]
Div8: [3, "CK_APB2 divided by 8"]
APB[12]PSC:
"APB[12]PSC":
Div1: [0, "CK_AHB"]
Div2: [4, "CK_AHB divided by 2"]
Div4: [5, "CK_AHB divided by 4"]
Expand Down
4 changes: 2 additions & 2 deletions peripherals/timer/timer_13_16_comcen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: MIT OR Apache-2.0

"TIMER1[3-6]":
CHCTL[01]_Output:
"TIMER1[3456]":
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@kaizensparc kaizensparc Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

"CHCTL[01]_Output":
"CH?COMCEN":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0COMCEN.CH0COMCEN"
2 changes: 1 addition & 1 deletion peripherals/timer/timer_16bit_f2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
CAR:
CAR: [0, 0xFFFF]

"TIMER[0-4],TIMER[7-9],TIMER1[0-3]":
"TIMER[01234789],TIMER1[0123]":
"CH?CV":
"CH?VAL": [0, 0xFFFF]
4 changes: 2 additions & 2 deletions peripherals/timer/timer_advanced_f2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

TIMER0:
CTL1:
"ISO[2-3]":
"ISO[23]":
_derivedFrom: "ISO0"
"ISO[1-2]N":
"ISO[12]N":
_derivedFrom: "ISO0N"
DMAINTEN:
CMTDEN:
Expand Down
24 changes: 1 addition & 23 deletions peripherals/timer/timer_basic.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

# Common features found on all timers.

TIMER0:
"TIMER*":
CTL0:
ARSE:
Disabled: [0, "The shadow register for CAR is disabled"]
Expand Down Expand Up @@ -34,27 +34,5 @@ TIMER0:
SWEVG:
UPG:
Update: [1, "Re-initializes the timer counter and generates an update of the registers."]

"TIMER[1-9],TIMER1[0-9]":
CTL0:
ARSE:
_derivedFrom: "TIMER0.CTL0.ARSE.ARSE"
UPS:
_derivedFrom: "TIMER0.CTL0.UPS.UPS"
UPDIS:
_derivedFrom: "TIMER0.CTL0.UPDIS.UPDIS"
CEN:
_derivedFrom: "TIMER0.CTL0.CEN.CEN"
DMAINTEN:
UPIE:
_derivedFrom: "TIMER0.DMAINTEN.UPIE.UPIE"
INTF:
UPIF:
_derivedFrom: "TIMER0.INTF.UPIF.UPIF"
SWEVG:
UPG:
_derivedFrom: "TIMER0.SWEVG.UPG.UPG"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@kaizensparc kaizensparc Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

@kaizensparc kaizensparc Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


"TIMER*":
PSC:
PSC: [0, 0xFFFF]
4 changes: 2 additions & 2 deletions peripherals/timer/timer_dma_f2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# SPDX-License-Identifier: MIT OR Apache-2.0

# TIMER8-13 doesn't have single pulse mode or DMA.
"TIMER[0-7]":
"TIMER[01234567]":
CTL0:
SPM:
Disabled: [0, "Counter is not stopped at update event"]
Expand All @@ -14,7 +14,7 @@
Enabled: [1, "Update DMA request enabled"]

# TIMER5/6 doesn't have DMA configuration registers or capture/compare.
"TIMER[0-4],TIMER7":
"TIMER[012347]":
CTL1:
DMAS:
OnCompare: [0, "CCx DMA request sent when CCx event occurs"]
Expand Down
106 changes: 17 additions & 89 deletions peripherals/timer/timer_general.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,31 @@
# TIMER5 is a basic timer, all the others are general or advanced, and so have compare mode (among
# other things).

# Use the same types for all timers, by deriving from one.
TIMER0:
"TIMER[01234]":
CTL0:
CKDIV:
Div1: [0, "t_DTS = t_CK_INT"]
Div2: [1, "t_DTS = 2 × t_CK_INT"]
Div4: [2, "t_DTS = 4 × t_CK_INT"]
DMAINTEN:
CH0IE:
"CH?IE":
Disabled: [0, "Capture/compare interrupt disabled"]
Enabled: [1, "Capture/compare interrupt enabled"]
"CH[1-3]IE":
_derivedFrom: "CH0IE"
INTF:
CH0OF:
"CH?OF":
Clear: [0, "No over capture occurred"]
OverCapture: [1, "A capture event occured while CHnIF was already set"]
"CH[1-3]OF":
_derivedFrom: "CH0OF"
CH0IF:
"CH?IF":
Clear: [0, "No capture or compare interrupt occurred"]
CaptureCompare: [1, "A capture or compare event occurred"]
"CH[1-3]IF":
_derivedFrom: "CH0IF"
SWEVG:
CH0G:
"CH?G":
CaptureCompare: [1, "Generate a capture or compare event"]
"CH[1-3]G":
_derivedFrom: "CH0G"
CHCTL0_Output:
CH0COMCEN:
"CHCTL?_Output":
"CH?COMCEN":
Disabled: [0, "Output compare clear disabled"]
Enabled: [1, "Output compare clear enabled"]
CH0COMCTL:
"CH?COMCTL":
Frozen:
[
0,
Expand All @@ -63,7 +54,7 @@ TIMER0:
"In upcounting, channel is active as long as CNT<CHyCV else inactive. In downcounting, channel is inactive as long as CNT>CHyCV else active",
]
PwmMode1: [7, "Inversely to PwmMode0"]
CH0COMSEN:
"CH?COMSEN":
Disabled:
[
0,
Expand All @@ -74,27 +65,16 @@ TIMER0:
1,
"Preload register on CHyCV enabled. Preload value is loaded into active register on each update event",
]
CH0COMFEN:
"CH?COMFEN":
Slow: [0, "The minimum delay from an edge is 5 clock cycles"]
Fast: [1, "The minimum delay from an edge is 3 clock cycles"]
CH0MS:
"CH?MS":
Output: [0, "Channel is configured as output"]
CI0: [1, "Channel is configured as input, ISx is connected to CI0FE0"]
CI1: [2, "Channel is configured as input, ISx is connected to CI1FE0"]
ITS: [3, "Channel is configured as input, ISx is connected to ITS"]
CHCTL[01]_Output:
"CH[1-3]COMCEN":
_derivedFrom: "CHCTL0_Output.CH0COMCEN.CH0COMCEN"
"CH[1-3]COMCTL":
_derivedFrom: "CHCTL0_Output.CH0COMCTL.CH0COMCTL"
"CH[1-3]COMSEN":
_derivedFrom: "CHCTL0_Output.CH0COMSEN.CH0COMSEN"
"CH[1-3]COMFEN":
_derivedFrom: "CHCTL0_Output.CH0COMFEN.CH0COMFEN"
"CH[1-3]MS":
_derivedFrom: "CHCTL0_Output.CH0MS.CH0MS"
CHCTL0_Input:
CH0CAPFLT:
"CHCTL?_Input":
"CH?CAPFLT":
NoFilter: [0, "Filter disabled. fSAMP=fDTS, N=1"]
TimerCk_N2: [1, "fSAMP=fTIMER_CK, N=2"]
TimerCk_N4: [2, "fSAMP=fTIMER_CK, N=4"]
Expand All @@ -111,72 +91,20 @@ TIMER0:
FDTS_Div32_N5: [13, "fSAMP=fDTS/32, N=5"]
FDTS_Div32_N6: [14, "fSAMP=fDTS/32, N=6"]
FDTS_Div32_N8: [15, "fSAMP=fDTS/32, N=8"]
CH0CAPPSC:
"CH?CAPPSC":
Div1: [0, "Prescaler disabled, capture on every edge"]
Div2: [1, "Capture every 2 edges"]
Div4: [2, "Capture every 4 edges"]
Div8: [3, "Capture every 8 edges"]
CH0MS:
"CH?MS":
Output: [0, "Channel is configured as output"]
CI0: [1, "Channel is configured as input, ISx is connected to CI0FEx"]
CI1: [2, "Channel is configured as input, ISx is connected to CI1FEx"]
ITS: [3, "Channel is configured as input, ISx is connected to ITS"]
"CHCTL[01]_Input":
"CH[1-3]CAPFLT":
_derivedFrom: "CHCTL0_Input.CH0CAPFLT.CH0CAPFLT"
"CH[1-3]CAPPSC":
_derivedFrom: "CHCTL0_Input.CH0CAPPSC.CH0CAPPSC"
"CH[1-3]MS":
_derivedFrom: "CHCTL0_Input.CH0MS.CH0MS"
CHCTL2:
CH0P:
"CH*P":
NotInverted: [0, "Active high"]
Inverted: [1, "Active low"]
"CH[1-3]P,CH?NP":
_derivedFrom: "CH0P"
CH0EN:
"CH?EN":
Disabled: [0, "Channel output is disabled"]
Enabled: [1, "Channel output is enabled"]
"CH[1-3]EN":
_derivedFrom: "CH0EN"

"TIMER[1-2],TIMER1[3-6]":
CTL0:
CKDIV:
_derivedFrom: "TIMER0.CTL0.CKDIV.CKDIV"
DMAINTEN:
"CH?IE":
_derivedFrom: "TIMER0.DMAINTEN.CH0IE.CH0IE"
INTF:
"CH?OF":
_derivedFrom: "TIMER0.INTF.CH0OF.CH0OF"
"CH?IF":
_derivedFrom: "TIMER0.INTF.CH0IF.CH0IF"
SWEVG:
"CH?G":
_derivedFrom: "TIMER0.SWEVG.CH0G.CH0G"
CHCTL[01]_Output:
"CH?COMCTL":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0COMCTL.CH0COMCTL"
"CH?COMSEN":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0COMSEN.CH0COMSEN"
"CH?COMFEN":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0COMFEN.CH0COMFEN"
"CH?MS":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0MS.CH0MS"
"CHCTL[01]_Input":
"CH?CAPFLT":
_derivedFrom: "TIMER0.CHCTL0_Input.CH0CAPFLT.CH0CAPFLT"
"CH?CAPPSC":
_derivedFrom: "TIMER0.CHCTL0_Input.CH0CAPPSC.CH0CAPPSC"
"CH?MS":
_derivedFrom: "TIMER0.CHCTL0_Input.CH0MS.CH0MS"
CHCTL2:
"CH?P,CH?NP":
_derivedFrom: "TIMER0.CHCTL2.CH0P.CH0P"
"CH?EN":
_derivedFrom: "TIMER0.CHCTL2.CH0EN.CH0EN"
"TIMER[1-2]":
CHCTL[01]_Output:
"CH?COMCEN":
_derivedFrom: "TIMER0.CHCTL0_Output.CH0COMCEN.CH0COMCEN"
6 changes: 3 additions & 3 deletions peripherals/timer/timer_interconnect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# Interconnection and slave mode.
# Timers 13-16 don't have interconnection.

"TIMER[0-2],TIMER14":
"TIMER[012],TIMER14":
SMCFG:
MSM:
NoSync: [0, "No action"]
Expand Down Expand Up @@ -45,14 +45,14 @@ TIMER0:
CompareO2C: [6, "O2CPRE signal is used as trigger output"]
CompareO3C: [7, "O3CPRE signal is used as trigger output"]

"TIMER[1-5],TIMER14":
"TIMER[12345],TIMER14":
CTL1:
MMC:
Reset: [0, "Use UPG bit from SWEVG register"]
Enable: [1, "Use CEN bit from CTL0 register"]
Update: [2, "Use the update event"]

"TIMER[0-2]":
"TIMER[012]":
CTL1:
TI0S:
Normal: [0, "The CH0 pin input is selected as channel 0 trigger input"]
Expand Down
6 changes: 3 additions & 3 deletions peripherals/timer/timer_interconnect_f2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# Interconnection and slave mode.
# Timers 13-16 don't have interconnection.

"TIMER[0-2],TIMER14":
"TIMER[012],TIMER14":
SMCFG:
MSM:
NoSync: [0, "No action"]
Expand Down Expand Up @@ -45,14 +45,14 @@ TIMER0:
CompareO2C: [6, "O2CPRE signal is used as trigger output"]
CompareO3C: [7, "O3CPRE signal is used as trigger output"]

"TIMER[1-5],TIMER14":
"TIMER[12345],TIMER14":
CTL1:
MMC:
Reset: [0, "Use UPG bit from SWEVG register"]
Enable: [1, "Use CEN bit from CTL0 register"]
Update: [2, "Use the update event"]

"TIMER[0-2]":
"TIMER[012]":
CTL1:
TI0S:
Normal: [0, "The CH0 pin input is selected as channel 0 trigger input"]
Expand Down
Loading