stm32_common/uart: add baudrate recalculation based on clk settings #8401
Conversation
drivers/include/periph/uart.h
Outdated
| * @return UART_OK on success | ||
| * @return UART_NOBAUD on inapplicable baudrate | ||
| */ | ||
| int uart_set_baudrate(uart_t uart, uint32_t baudrate); |
There was a problem hiding this comment.
I think this API introduction would need some discussion, but for now I'd we should call it uart_calc_baudrate or something like that. I'm not sure how many platforms support this feature but I'd say to put this function as static in the stm32l code.
There was a problem hiding this comment.
Since this function is introduced to be used in pm.h, if it's static it wouldn't be available. I could just declare it here and as extern in pm.h, would that work? doesn't seem very clean though.
There was a problem hiding this comment.
You don't need to add it to the API but only as a local function for the stm32 family let's say. You can put it in cpu/stm32_common/include/periph_cpu_common.h
cpu/stm32_common/periph/uart.c
Outdated
| mantissa = (uint16_t)(uart_clk / 16); | ||
| fraction = (uint8_t)(uart_clk - (mantissa * 16)); | ||
| dev(uart)->BRR = ((mantissa & 0x0fff) << 4) | (fraction & 0x0f); | ||
| #endif |
There was a problem hiding this comment.
Can you explain all the magic numbers in this function? The hex are understandable but I'd prefer to see the 8 and 16 as constants like #define UART_8X_OVERSAMPLING 8, or something like that.
|
Please prefix your commit too. |
|
@kYc0o Changes addressed. |
kYc0o
added
Type: enhancement
| * @return UART_OK on success | ||
| * @return UART_NOBAUD on inapplicable baudrate | ||
| */ | ||
| int uart_set_baudrate(unsigned int uart, uint32_t baudrate); |
There was a problem hiding this comment.
This should be periph_uart_set_baudrate (prefixed with the module name).
|
NACK from my side. As far as I can see, this PR breaks with a number design concepts in RIOT! Mainly: our power management system is based around the concept, that CPU peripherals block certain sleep modes, as long as these peripherals are active. So in the case of UART: as the UART on STMs is not working in deep sleep modes, these should be blocked as long as the uart is enabled (I know, its not implemented, yet). And if we re-enable a UART after we have been to deep sleep, there is AFAIK no need to re-initialize the baudrate, right? Side note: this PR is also introducing a lot of unnecessary overhead and breaks the original UART code in some positions... |
haukepetersen
added
the
Discussion: contested
For reference, I've started to work on pm for stm32, see #7787 |
And it will never be, I hope. Because implementing this means LPM modes are totally useless — the very meaning of STOP mode is to stop CPU core and peripheral devices. Manually disabling all the peripherals (UART, I2C, SPI, timers, etc., etc., etc.) before entering STOP/SLEEP makes no sense at all to me.
It depends. Proper LPM implementation should allow the CPU to switch core frequency on the fly. |
I had some thoughts around the topic and changed my mind. We currently don't have a real complete concept, but parts of it here and there. Because of implementing that way, this CPU was working sometimes just by miracle and by consequence being really fragile. I started to look at this PR and said "well, someone made the effort, if it works properly it can be merged regardless of the obvious difference in the concepts". But as long as it doesn't break other CPUs for me it was ok. I agree about consistency between CPU implementations, but PM in this case is something that IMHO cannot be that much generalised, especially taking into account the way low power CPUs are designed. There is an obvious chart at least for ST CPUs on which they state for which purposes the CPUs are designed, and thus they IMHO might be implemented differently. TL;DR: Low power CPUs need a different concept IMHO, since they work very different compared to e.g. high performance CPUs. |
|
Oh oh, seems I triggered a more fundamental discussion here :-) It seems to me that we have quite different perceptions about RIOT's current PM system.
Thanks for the honest opinion :-). So why are they useless? Maybe it makes sense to name some specific use cases, where the current PM does not work?!
|
Here's the discussion, Long story short, shell will become completely incompatible with sleep modes — UART will prevent system from going to sleep, shell can not be stopped, UART can not be disabled while shell is running. With current code (without pm_layered) I can enable and disable LPM whenever I like to. With proposed changes I can't. Between can't and can I prefer latter.
Oh, yes it is. :) We did it some time ago with our firmwares, it's useful sometimes — when you need to wait for some external event without going to sleep (because of wakeup time) and you want reduced power consumption. Full manual control, as for now, but I'm thinking of integrating this in pm_layered actually. There's no added complexity if you don't need it — don't use it. |
True. Moreover, even different low-power MCUs may require different approach. Take a look on TI CC1310, it has 3 CPU cores — two Cortexes and one 16-bit Sensor Controller. Generalized approach just doesn't work here. |
|
Let's continue the general discussion in #8428. |
|
see replys in #8428 |
|
Wow many subjects to address here jajaja. First regarding your first comments @haukepetersen, in the current state of the related PR, it is true this not really needed. Currently it's there because the option to switch to msi clock in #8402 enables someone to use it somewhere else, which would mean it would have to recalculate uart baudrate accordingly to that new speed. But thinking about it now that would also mean recalculating timer and another bunch of clock related periph that haven't been included in any of these PR's. So this could in fact be dropped (would have to test but it think so), unless some of it's refactor is of any use, but if you feel it breaks I have no problem with dropping this. And in #8402 msi_clock feature still needs to be kept since it's needed for sleep mode. That being said the real reason this was first introduced was because at the original state the "low power modes" implemented for stm32l1 weren't only sleep modes but also low power run modes. These modes required switching to MSI clock, with in turn required recalculating baudrate, timer config, etc. I ended up cutting them because RIOT only defines 4 low power modes (3 discarding normal run) (why only 4 BTW??) and because it would mean a lot of contestable/controversial api changes in other peripherals just as this one has been.
This part surprises me a little. In most use cases some kind of low power is needed, which isn't the case for most RIOT applications, since is mostly not implemented in RIOT. So i guess it depend on what application you are targeting for RIOT, which is again another discussion. No going back to the current definitions of PM. IMHO I feel it's currently quite limited. In my personal experience and what I have seen in other projects that haven been deployed using RIOT, people (including me) have to implement all/most low power features or tools to be able to actually handle this (RTC features, low power modes for radios, etc.), which a lot of time means a lot of hacking around because a lot of RIOT features aren't though for low-power. Maybe @olegart has different thoughts on this, and he also has experience (and more than me) in developing products around RIOT. But again that's another discussion for another thread, and I'm not really informed on RIOT's feature goals regarding low-power. |
|
@fjmolinas I was about to copy your reply to #8428, but that would put it out of context. Would you restate your critique on pm in that issue please? |
|
@fjmolinas regarding this specific PR: I see the point of adding the possibility of adapting the UART baudrate dynamically. But it feels quite hacky to me, so simply add this function for now and see where/when it is used. So if we really add the possibility to change the core clock dynamically for this CPU, then I'd say it would be much cleaner to embed this change into a (CPU-)global concept on what do do when the clock speed changes. So what to do with all the other peripherals (i2c, spi, timers, ...), how to only apply clock changes to actually enabled peripherals, etc... regarding the general points: see #8428 |
|
@kYc0o @haukepetersen thinking of droping this, not really needed anymore for #8403. If runtime clock configuration is added later on it can be re-opened or implemented better. But for now it would just be unused code. |
|
+1. I'm doing exhaustive tests on nucleo-l152 and found several issues which would take time to address. @haukepetersen feel free to close it if you agree. |
|
@kYc0o @haukepetersen closed this one |
Contribution description
This PR is based on work by @olegart, it adds the possibility to recalculate baud rate based on clock frequency, needed for proper re-initialization on devices after entering low power modes.
Issues/PRs references
#8403 depends on this PR