stm32g0: Expand stm32g0 family

[leptun]

Added stm32g070, stm32g071 and stm32g0b0 support.

This PR should make sure that the stm32g0b0 uses TIM3 instead of TIM2 (eg, see BTT Manta M4P which might have a stm32g0b0ret6 where TIM2 doesn't exist). I also expanded the support to stm32g070 and stm32g071 since those were the only nucleo-g0 boards I found in stock at the moment. I'm still waiting for them to be delivered, so I haven't tested this PR yet unfortunately. I'm submitting it early so that I may get some feedback on the code in advance (marked as draft for now). If anyone has a board with stm32g0b0 and can test this, I would be grateful.

Signed-off-by: Alex Voinea [email protected]

[leptun]

I've checked and it works on STM32G070RB and STM32G071RB. Can't really check the G0Bx variants since I can't get my hands on such a board.

[KevinOConnor]

Thanks. In general, I'm fine with adding support for additional boards like this. However, this commit adds several "ifdefs" to the code - some of which look incorrect (eg, the stm32f0_i2c.c and stm32f0_serial.c). I wonder if there is a way to do this with less ifdefs.

Separately, will the stm32g0b1 binary run unmodified on an stm32g071 chip?

-Kevin

[leptun]

Separately, will the stm32g0b1 binary run unmodified on an stm32g071 chip?

It will not because of the different SRAM size. But otherwise it should work as long as the peripherals used exist also there (some don't such as CAN and some I2C)

I wonder if there is a way to do this with less ifdefs.

I'm not sure how I could do it with less ifdefs. In any case, I've been using the stm32g071rb as my X/Y axis microcontroller for the past week and I haven't had issues with that. I'm using it over UART (the default nucleo board interface)

[github-actions[bot]]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[leptun]

@KevinOConnor I've rebased the changes to the latest master. I'm still not entirely sure what I need to change so that this PR can get merged.

However, this commit adds several "ifdefs" to the code - some of which look incorrect (eg, the stm32f0_i2c.c and stm32f0_serial.c). I wonder if there is a way to do this with less ifdefs.

I'm not entirely sure what I could do about all these ifdefs. It's disappointing that the chips differ in this way, but I don't see a way to have those features besides using ifdefs. The only change I can think of is instead of doing for example #ifdef USB_BASE I use #if CONFIG_xxx, but I'm not sure if that will result in cleaner code.

[KevinOConnor]

Some of the changes here are definitely incorrect - for example, a change to the i2c_bus ifdefs in stm32f0_i2c.c must have a corresponding change to the ifdefs in the DECL_ENUMERATION part of that file (and the enumeration number must exactly match the i2c_bus array offset).

Separately, will the stm32g0b1 binary run unmodified on an stm32g071 chip?

It will not because of the different SRAM size.

If you set the ram size of the stm32g0 build to 0x9000 does it then work? Klipper never uses more than about 16KiB of ram, so we can reduce the size of the existing stm32g0 definition if that helps the code run on more chips.

-Kevin

[leptun]

If you set the ram size of the stm32g0 build to 0x9000 does it then work? Klipper never uses more than about 16KiB of ram, so we can reduce the size of the existing stm32g0 definition if that helps the code run on more chips.

Reducing the RAM size should make it work, BUT, there is still the problem where the lower tier G0 MCUs lack features (or are not supposed to be used). Here are the capabilities:

MCU	SRAM	USB	CAN	I2C3	TIM4	TIM2 (system timer)
STM32G0B1	144KB (128KB with parity enabled)	yes	yes	yes	yes	yes
STM32G0B0	144KB (128KB with parity enabled)	yes	no (!)	yes	yes	no (should use 16bit TIM3)
STM32G071	36KB (32KB with parity enabled)	no	no	no	no	yes
STM32G070	36KB (32KB with parity enabled)	no	no	no	no	no (should use 16bit TIM3)

I'm not sure what might happen if for example a G0B1 binary tried to use the USB/CAN on the G070. It can either just work normally (except for the USB/CAN themselves) or the firmware could crash because those peripherals don't exist. In any case, I'll reduce the RAM size of G0Bx to match G07x if you say this helps with consistency. I would have personally kept them like this since I don't like wasted resources, but I guess klipper will not exceed 36KB of RAM any time soon.

Some of the changes here are definitely incorrect - for example, a change to the i2c_bus ifdefs in stm32f0_i2c.c must have a corresponding change to the ifdefs in the DECL_ENUMERATION part of that file (and the enumeration number must exactly match the i2c_bus array offset).

Oh, I see what you mean. I forgot about the declarations above. I'm not entirely sure how I could order these so that it doesn't become a preprocessor nightmare. Because if I2C3 is missing, then the I2C2 definitions will end up shifting down in index. Would it make sense to split the DECL_ENUMERATION blocks per MCU family? So for example CONFIG_MACH_STM32G0 and CONFIG_MACH_STM32L4 would not be combined. With this I'd also move the common (F0/G0/L4) I2C definitions to the beginning instead of at the end of i2c_info.

[KevinOConnor]

Yes - certainly the g0x0 variants will need different support. For some reason I thought most of the ifdefs were due to the g0b1 vs g071 - is that not the case?

-Kevin

[leptun]

Yes - certainly the g0x0 variants will need different support. For some reason I thought most of the ifdefs were due to the g0b1 vs g071 - is that not the case?

The main reason for the huge number of defines is because of the G07x series. The G0B0 alone is not that hard to implement alongside the G0B1, but the G07x series requires quite a bit more defines for them to work.

I'm still torn whether I should reduce the ram of the G0Bx series. It's not like you would want to use a G0Bx series binary on the G07x series microcontrollers, so unifying the memory usage doesn't yield any real improvement in code quality. It just makes a smaller diff, but at the cost of having a misleading RAM_SIZE config value. As for 0x0 vs 0x1 series, those are not interchangeable at all due to the timer code. A 0x0 binary should work on the 0x1, but not the other way around. And even so, it is preferable to have a 32bit system timer, not a software extended 16bit timer. I've pushed an update for the I2C code. I hope this is correct now. I also had to rebase on top of the STM32G4 changes. Please let me know what else I should change to push this PR forward.

Speaking of the STM32G4, I also pushed a fix for the SWD accidental disable on that mcu family. However, I'm not entirely sure that this was a problem in the first place since I don't have that G4 model MCU avaliable to test. All things considered, the situation looked very similar to the STM32G0, so I ported the fix to that MCU family as well.

[KevinOConnor]

Okay, thanks for the clarification.

The G0B0 alone is not that hard to implement alongside the G0B1

Would it be possible to break out the G0B0 code into a separate commit (and/or PR) then? I know that's more work, but it would make review easier. I'm still a little leery about the ifdefs (mainly because I don't want to introduce a regression on one of the other stm32 targets). I don't see an issue with adding ifdefs if that is the best way to support the 07x chips, but it would be easier to review them if it's just those chips (and not 0bx).

-Kevin

[leptun]

Sure. I'll make a separate PR for that in the coming days. Should I include the G0 and G4 SWD bugfix also?

[KevinOConnor]

Yes - please make the swd fix separate and ping @mattthebaker on it.

-Kevin

[mattthebaker]

The G4 SWD change looks fine, that would explain why I needed to make openocd hold the device in reset before initiating programming. I didn't actually need to use the debug interface for debug or I would have noticed this. Future boards are less likely to have resetb wired to the debug header so this should save someone some trouble.

The i2c DECL statements for L4 look like they have different ordering than the structure in the latest commit.

[mattthebaker]

The i2c configurations for L4 look good now. 👍

[leptun]

@mattthebaker Thanks. @KevinOConnor I think the PR is ready again

[KevinOConnor]

Thanks. The changes to stm32f0_i2c.c don't look correct though. The ifdefs in the pin declarations should be identical to the ifdefs in the array setup - otherwise it becomes a maintenance nightmare (or at least, more of one).

-Kevin

[leptun]

I've pushed a commit addressing the requests. I did not have time to test them however, but I think it should be fine as it still compiles.

[KevinOConnor]

Thanks.

-Kevin