stm32-rs / stm32g0xx-hal

Peripheral access API for STM32G0 series microcontrollers
Apache License 2.0
73 stars 51 forks source link

feature stm32g07x should be split up #18

Closed ijager closed 4 years ago

ijager commented 4 years ago

Currently we have the following features

stm32g07x = ["stm32g0/stm32g07x"]
stm32g030 = ["stm32g0/stm32g030"]
stm32g031 = ["stm32g0/stm32g031"]
stm32g041 = ["stm32g0/stm32g041"]
stm32g081 = ["stm32g0/stm32g081"]

However there are many distinctions between stm32g070 and stm32g071, so I think we should add separate features for both.

Also we might end up doing the following all over:

#[cfg(not(any(
    feature = "stm32g070",
    feature = "stm32g030"
)))]
// Datasheet DS12991 Rev 3 3.14.1
const TS_CAL2: *mut u16 = 0x1FFF_75CA as *mut u16;

So maybe we need to have a feature for value line / non-value line?

andresv commented 4 years ago

Indeed ST itself differentiates G0 line as G0x0 value line and G0x1 lets say normal line. If separating those lines makes adding new features easier I am happy with that. @dotcypress what is your opinion on that?

ijager commented 4 years ago

The peripheral access crate claims to also make that distinction:

Module Devices Links
stm32g0x0 STM32G070, STM32G030 RM0454, st.com
stm32g0x1 STM32G031, STM32G041, STM32G071, STM32G081 RM0444, st.com

However, internally it has the stm32g07x module which includes stm32g71 features. So maybe this needs to be fixed at the source?

andresv commented 4 years ago

This comes from the original SVD files: https://github.com/stm32-rs/stm32-rs/blob/master/svd/vendor/en.stm32g0_svd.zip. There are:

stm32g07x.svd
stm32g030.svd
stm32g031.svd
stm32g041.svd
stm32g081.svd

I ping @adamgreig, he could have an opinion for that issue.

ijager commented 4 years ago

I kinda understand now how things went wrong, because the ST documentation is super confusing and conflicting.

For example this is the feature list from the stm32g0x0 value line manual RM0454 rev 2 Section 1.4 Feature STM32G030 STM32G070
DAC No Yes
COMP No Yes
TIM6 and TIM7 No Yes
TIM15 No Yes
UCPD No Yes
CEC No Yes

However, the STM32G070 does not have a DAC, COMP, UCPD and HDMI CEC according to the Product Selector. Also there are no chapters about those peripherals in the same manual.

This has now been updated in Rev 3 I just found out..

So the difference between STM32G070 and STM32G071 seems to be those peripherals: DAC, Comparator, HDMI CEC and USB PD, in addition to the common STM32G0x1 features.

I am not sure what source of truth to rely on.

The SVD files in stm32-rs also indicate there is something not quite right:

~/dev/rust/stm32-rs/svd$ ls -hl stm32g0*.svd                                                                                                                                                                                                                                                  master
742K Aug 29  2019 stm32g030.svd
723K Aug 29  2019 stm32g031.svd
742K Aug 29  2019 stm32g041.svd
897K Aug 29  2019 stm32g07x.svd
897K Aug 29  2019 stm32g081.svd

~/dev/rust/stm32-rs/svd$ diff stm32g030.svd.patched stm32g041.svd.patched                                                                                                                                                                                                                master
2c2
<   <name>STM32G030</name>
---
>   <name>STM32G041</name>
4c4
<   <description>STM32G030</description>
---
>   <description>STM32G041</description>

~/dev/rust/stm32-rs/svd$ diff stm32g07x.svd.patched stm32g081.svd.patched                                                                                                                                                                                                            1 ↵ master
2,4c2,4
<   <name>STM32G07X</name>
<   <version>1.3</version>
<   <description>STM32G07x</description>
---
>   <name>STM32G081</name>
>   <version>1.0</version>
>   <description>STM32G081</description>

There are no differences betweenstm32g030.svd and stm32g041.svd. Same for stm32g07x.svd and stm32g081.svd.

adamgreig commented 4 years ago

Jeez, you'd have hoped these new devices might have good SVD files! It seems most like the existing SVD files are just wrong, since of course there are differences between the G030 and G041. It's possible there might be newer SVDs available but I can't find them on the ST website.

I'm perfectly happy to update the devices we have in stm32-rs to a more sensible set if that makes sense. It seems reasonable for the HAL to have the same features as the stm32g0 crate, but if the underlying SVDs are identical then there's no point having different features at all. It might just be that we need to fix those SVD files to reflect the actual hardware more. In other families we've got some devices where the same SVD file is copied twice and then modified differently to accurately reflect the hardware.

On the PAC documentation front, our supported devices table is definitely wrong; in https://github.com/stm32-rs/stm32-rs/commit/c647f3ef1989fcad1fa75e34a9145b500045694b (which is when we changed from just g0x0 and g0x1 to having all the current devices) it should have been updated to list all the new modules but instead just had the new devices added to the existing module names. So either way that will need fixing.

ijager commented 4 years ago

To come back to the original issue. I made an overview of some peripherals that are not shared among all devices. And there is not really a clear split. It does not make sense to group G070 and G071 together. But also grouping on G0X0/G0X1 is not really an option.

So I think we just need to have a feature for each separate part. Looking at stm32L0xx-hal they have features for every single part and also packages. So I think we cannot really avoid it.

Feature Desc G030 G070 G031 G041 G071 G081
TIM1 16bit up/down 1 1 1 1 1 1
TIM2 32bit up/down     1 1 1 1
TIM3 16bit up/down 1 1 1 1 1 1
TIM6 16bit up   1     1 1
TIM7 16bit up   1     1 1
TIM14 16bit up 1 1 1 1 1 1
TIM15 16bit up   1     1 1
TIM16 16bit up 1 1 1 1 1 1
TIM17 16bit up 1 1 1 1 1 1
LPTIM1 16bit up     1 1 1 1
LPTIM2 16bit up     1 1 1 1
COMP1 Comparator         1 1
COMP2 Comparator         1 1
DAC           1 1
UART1   1 1 1 1 1 1
UART2   1* 1 1* 1* 1 1
UART3     1     1 1
UART4     1     1 1
LPUART       1 1 1 1
AES             1
RNG             1
UCPD1 USB C Power Delivery         1 1
UCPD2 USB C Power Delivery         1 1
VREFBUF       1 1 1 1
TS_CAL2 Tsense calibration val @ 130 C     1 1 1 1
DMA Channels 5 7 5 5 7 7
CEC HDMI control         1 1
andresv commented 4 years ago

Excellent overview! I should add this table to README.md with feature flags.

@ijager did you also check packages, are these features available with all combinations? @adamgreig what do you think, should we go to L0 route? I try to keep it similar to at least some family, it makes future maintenance easier.

dotcypress commented 4 years ago

I'm fully agree with @adamgreig : This is all about finding correrct SVD files

andresv commented 4 years ago

Fix merged.