Fix STM32 adc common #2717

cpunion · 2022-03-16T02:24:49Z

Currently, some generated files from STM32's svd file have some problems:

ADC* and ADC_Common has the same type, because they have the same group name ADC, ADC_Common should has a different type. For example in src/device/stm32/stm32f429.go:

	// Analog-to-digital converter
	ADC1 = (*ADC_Type)(unsafe.Pointer(uintptr(0x40012000)))

	......

	// Common ADC registers
	ADC_Common = (*ADC_Type)(unsafe.Pointer(uintptr(0x40012300)))

ADC_Type used ADC_Common's structure, because ADC_Common be described before ADC* in the svd file, for example in src/device/stm32/stm32f410.go:

// ADC common registers
type ADC_Type struct {
	CSR volatile.Register32 // 0x0
	CCR volatile.Register32 // 0x4
}

Some ADC_Common used wrong case like ADC_common, for example in src/device/stm32/stm32mp153.svd:

<name>ADC_common</name>

This PR just fix those problems before a better way.

cpunion · 2022-03-17T03:34:32Z

Difference of generated files:

 stm32f215.go  |  462 +++++++++++++-
 stm32f217.go  |  462 +++++++++++++-
 stm32f401.go  | 1205 +++++++++++++++++++++++++++++++++++-
 stm32f401.go  | 1205 +++++++++++++++++++++++++++++++++++-
 stm32f405.go  |  462 +++++++++++++-
 stm32f407.go  |  462 +++++++++++++-
 stm32f410.go  | 1563 ++++++++++++++++++++++++++++++++++++++++++++---
 stm32f410.go  | 1563 ++++++++++++++++++++++++++++++++++++++++++++---
 stm32f411.go  | 1205 +++++++++++++++++++++++++++++++++++-
 stm32f411.go  | 1205 +++++++++++++++++++++++++++++++++++-
 stm32f412.go  | 1563 ++++++++++++++++++++++++++++++++++++++++++++---
 stm32f412.go  | 1563 ++++++++++++++++++++++++++++++++++++++++++++---
 stm32f413.go  |  165 ++++-
 stm32f427.go  |  462 +++++++++++++-
 stm32f429.go  |  462 +++++++++++++-
 stm32f446.go  |  462 +++++++++++++-
 stm32f469.go  |  462 +++++++++++++-
 stm32f730.go  |  462 +++++++++++++-
 stm32f745.go  |  462 +++++++++++++-
 stm32f750.go  |  462 +++++++++++++-
 stm32f765.go  |  462 +++++++++++++-
 stm32f7x2.go  |  462 +++++++++++++-
 stm32f7x3.go  |  462 +++++++++++++-
 stm32f7x6.go  |  462 +++++++++++++-
 stm32f7x7.go  |  462 +++++++++++++-
 stm32f7x9.go  |  462 +++++++++++++-
 stm32l4r9.go  |  407 ++++++++++++-
 stm32l4x1.go  |  407 ++++++++++++-
 stm32l4x2.go  |  407 ++++++++++++-
 stm32l4x3.go  |  407 ++++++++++++-
 stm32l4x5.go  |  397 +++++++++++-
 stm32l4x6.go  |  407 ++++++++++++-
 stm32l552.go  | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 stm32l552.go  | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 stm32l562.go  | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 stm32l562.go  | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 stm32mp153.go |  422 ++++++++++++-
 stm32mp157.go |  422 ++++++++++++-
 stm32wb55.go  |  208 ++++++-
 33 files changed, 20282 insertions(+), 541 deletions(-)

Different contents:
stm32-gen.diff.zip

aykevl · 2022-03-19T18:29:36Z

I don't fully understand the problem. Is there anything that can't be done at the moment or where the current ADC values (ADC1, ADC2, ...) have an incorrect structure?

Also, I don't think this is the correct fix. If the SVD file is incorrect, the SVD file should be fixed (see https://github.com/stm32-rs/stm32-rs). If the gen-device-svd tool is incorrect, this bug should be fixed properly and not using a hack.

cpunion · 2022-03-20T03:25:14Z

I don't fully understand the problem. Is there anything that can't be done at the moment or where the current ADC values (ADC1, ADC2, ...) have an incorrect structure?

Some of generated files have a wrong ADC_Type, e.g. stm32f410.go:

// ADC common registers
type ADC_Type struct {
	CSR volatile.Register32 // 0x0
	CCR volatile.Register32 // 0x4
}

The correct version is like this:

// Analog-to-digital converter
type ADC_Type struct {
	SR    volatile.Register32 // 0x0
	CR1   volatile.Register32 // 0x4
	CR2   volatile.Register32 // 0x8
	SMPR1 volatile.Register32 // 0xC
	SMPR2 volatile.Register32 // 0x10
	JOFR1 volatile.Register32 // 0x14
	JOFR2 volatile.Register32 // 0x18
	JOFR3 volatile.Register32 // 0x1C
	JOFR4 volatile.Register32 // 0x20
	HTR   volatile.Register32 // 0x24
	LTR   volatile.Register32 // 0x28
	SQR1  volatile.Register32 // 0x2C
	SQR2  volatile.Register32 // 0x30
	SQR3  volatile.Register32 // 0x34
	JSQR  volatile.Register32 // 0x38
	JDR1  volatile.Register32 // 0x3C
	JDR2  volatile.Register32 // 0x40
	JDR3  volatile.Register32 // 0x44
	JDR4  volatile.Register32 // 0x48
	DR    volatile.Register32 // 0x4C
}

And most of generated files has a wrong ADC_Common:

// ADC common registers
ADC_Common = (*ADC_Type)(unsafe.Pointer(uintptr(0x40012300)))

It should be

ADC_Common = (*ADC_Common_Type)(unsafe.Pointer(uintptr(0x40012300)))

Or

ADC = (*ADC_Common_Type)(unsafe.Pointer(uintptr(0x40012300)))

And it should has the type:

// ADC common registers
type ADC_Common_Type struct {
	CSR volatile.Register32 // 0x0
	CCR volatile.Register32 // 0x4
        // ....
}

See the part of the header file of CMSIS below.

Also, I don't think this is the correct fix. If the SVD file is incorrect, the SVD file should be fixed (see https://github.com/stm32-rs/stm32-rs). If the gen-device-svd tool is incorrect, this bug should be fixed properly and not using a hack.

You are right, may we can fix it in stm32-rs or https://github.com/tinygo-org/stm32-svd.
stm32-rs uses svd2rust to generate types like below (pseudo code):

type ADC_COMMON struct
type ADC1 struct
type ADC2 = ADC1 // ADC2 `derivedFrom` ADC1
type ADC3 = ADC1 // ADC3 `derivedFrom` ADC1
...

I think they doesn't care the type names.

In the header file of CMSIS, it defines them like below:

/** 
  * @brief Analog to Digital Converter  
  */

typedef struct
{
  __IO uint32_t SR;     /*!< ADC status register,                         Address offset: 0x00 */
  __IO uint32_t CR1;    /*!< ADC control register 1,                      Address offset: 0x04 */
  __IO uint32_t CR2;    /*!< ADC control register 2,                      Address offset: 0x08 */
  __IO uint32_t SMPR1;  /*!< ADC sample time register 1,                  Address offset: 0x0C */
  __IO uint32_t SMPR2;  /*!< ADC sample time register 2,                  Address offset: 0x10 */
  __IO uint32_t JOFR1;  /*!< ADC injected channel data offset register 1, Address offset: 0x14 */
  __IO uint32_t JOFR2;  /*!< ADC injected channel data offset register 2, Address offset: 0x18 */
  __IO uint32_t JOFR3;  /*!< ADC injected channel data offset register 3, Address offset: 0x1C */
  __IO uint32_t JOFR4;  /*!< ADC injected channel data offset register 4, Address offset: 0x20 */
  __IO uint32_t HTR;    /*!< ADC watchdog higher threshold register,      Address offset: 0x24 */
  __IO uint32_t LTR;    /*!< ADC watchdog lower threshold register,       Address offset: 0x28 */
  __IO uint32_t SQR1;   /*!< ADC regular sequence register 1,             Address offset: 0x2C */
  __IO uint32_t SQR2;   /*!< ADC regular sequence register 2,             Address offset: 0x30 */
  __IO uint32_t SQR3;   /*!< ADC regular sequence register 3,             Address offset: 0x34 */
  __IO uint32_t JSQR;   /*!< ADC injected sequence register,              Address offset: 0x38*/
  __IO uint32_t JDR1;   /*!< ADC injected data register 1,                Address offset: 0x3C */
  __IO uint32_t JDR2;   /*!< ADC injected data register 2,                Address offset: 0x40 */
  __IO uint32_t JDR3;   /*!< ADC injected data register 3,                Address offset: 0x44 */
  __IO uint32_t JDR4;   /*!< ADC injected data register 4,                Address offset: 0x48 */
  __IO uint32_t DR;     /*!< ADC regular data register,                   Address offset: 0x4C */
} ADC_TypeDef;

typedef struct
{
  __IO uint32_t CSR;    /*!< ADC Common status register,                  Address offset: ADC1 base address + 0x300 */
  __IO uint32_t CCR;    /*!< ADC common control register,                 Address offset: ADC1 base address + 0x304 */
  __IO uint32_t CDR;    /*!< ADC common regular data register for dual
                             AND triple modes,                            Address offset: ADC1 base address + 0x308 */
} ADC_Common_TypeDef;

//....

#define ADC                 ((ADC_Common_TypeDef *) ADC_BASE)
#define ADC1                ((ADC_TypeDef *) ADC1_BASE)
#define ADC2                ((ADC_TypeDef *) ADC2_BASE)
//...

It has only two types names ADC_Common_TypeDef and ADC_TypeDef, I think it is more readable.

Maybe we can add a rewrite step in the stm32-svd project's Makefile to generate a good group name, and then gen-device-svd would generate the correct and readable type names(ADC_Common_Type and ADC_Type).

cpunion · 2022-04-17T11:08:00Z

@aykevl Could you please help to review the PR stm32-rs/stm32-rs#719 ?

719: Change the groupName of ADC_Common from ADC to ADC_Common r=adamgreig a=cpunion In the https://github.com/tinygo-org/stm32-svd project, it depends this project and generates types by the peripheral's groupName. Because the ADC_Common's groupName is ADC, it can't generate the types ADC and ADC_Common correctly. I want to change the ADC_Common's groupName to ADC to make it work fine. See tinygo-org/tinygo#2717 , tinygo-org/stm32-svd#7 , but those are not good way. I checked the generated rust code, it's not changed after apply this change and run make, seems it doesn't depend groupName. So can you accept this PR? Co-authored-by: Li Jie <cpunion@gmail.com>

719: Change the groupName of ADC_Common from ADC to ADC_Common r=adamgreig a=cpunion In the https://github.com/tinygo-org/stm32-svd project, it depends this project and generates types by the peripheral's groupName. Because the ADC_Common's groupName is ADC, it can't generate the types ADC and ADC_Common correctly. I want to change the ADC_Common's groupName to ADC to make it work fine. See tinygo-org/tinygo#2717 , tinygo-org/stm32-svd#7 , but those are not good way. I checked the generated rust code, it's not changed after apply this change and run make, seems it doesn't depend groupName. So can you accept this PR? Co-authored-by: Li Jie <cpunion@gmail.com> Co-authored-by: Adam Greig <adam@adamgreig.com>

fix ADC_Common type

a033d85

cpunion changed the base branch from release to dev March 16, 2022 02:25

Fix name conversion for "*ADC_Common*"

79f01b4

cpunion mentioned this pull request Mar 20, 2022

Change ADC_Commont's group name from ADC to ADC_Common tinygo-org/stm32-svd#7

Closed

cpunion closed this Mar 20, 2022

cpunion mentioned this pull request Apr 17, 2022

Change the groupName of ADC_Common from ADC to ADC_Common stm32-rs/stm32-rs#719

Merged

cpunion mentioned this pull request Jul 6, 2022

Update to stm32-rs 958ae32 (v0.15.1) tinygo-org/stm32-svd#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix STM32 adc common #2717

Fix STM32 adc common #2717

Uh oh!

cpunion commented Mar 16, 2022 •

edited

Loading

Uh oh!

cpunion commented Mar 17, 2022

Uh oh!

aykevl commented Mar 19, 2022

Uh oh!

cpunion commented Mar 20, 2022 •

edited

Loading

Uh oh!

cpunion commented Apr 17, 2022

Uh oh!

Uh oh!

Fix STM32 adc common #2717

Fix STM32 adc common #2717

Uh oh!

Conversation

cpunion commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpunion commented Mar 17, 2022

Uh oh!

aykevl commented Mar 19, 2022

Uh oh!

cpunion commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpunion commented Apr 17, 2022

Uh oh!

Uh oh!

cpunion commented Mar 16, 2022 •

edited

Loading

cpunion commented Mar 20, 2022 •

edited

Loading