Hi Karl, I'm not sure what you see as a regression in the API or step backwards. Everything should work exactly the same way as before if user won't define any LSI_FREQUENCY at all (except for those slight changes in rounding).
The fact that every device may have different LSI frequency is the reason why user should be allowed to adjust LSI frequency. Note that LSI_FREQUENCY was not exposed at all before (it was just defined in .c with no use). I don't even understand what does LSE have to do with this. Is there any device which can clock IWDG from LSE? Could you clarify in what situation would it behave differently from what user would expect? Or how would it break any existing code? If you just want to expose some LSI_FREQUENCY_NOMINAL macro which would stay the same even if user defined his own LSI_FREQUENCY it shouldn't be a problem though I'm not sure what purpose would it serve. I certainly wouldn't configure anything clocked from LSE based on some macro named LSI_*. Filip On Tue, Oct 09, 2018 at 07:39:47PM -0000, Karl Palsson wrote: > > This seems to be a regression in the API just so it can appear > easier for people using LSI to get the "right" time. > > Remember, LSI is specified as nominal, but for instance, on the > L1, it's nominally 38kHz, but is anywhere from 26 to 56. It's not > even _remotely_ viable to try and do things in explicit ticks if > you're using LSI rather than LSE. > > Juggling headers to make the nominal include of LSI a bit better > is definitely a bugfix, but removing the actually accurate ticks > for people using LSE in favour of this seems like a step > backwards. > > Cheers, > Karl P > > > Filip Moc <d...@moc6.cz> wrote: > > And create iwdg_set_period_ms in header file as a wrapper for > > iwdg_set_period_ticks. > > > > This allows to define one's own LSI_FREQUENCY before including > > iwdg.h to adjust iwdg period calculations to LSI clock of real > > device. > > > > Also iwdg_set_period_ticks now rounds period up to nearest > > possible value rather than truncating it down. > > > > Signed-off-by: Filip Moc <d...@moc6.cz> > > --- > > > > Notes: > > I changed semantics of count & prescale calculations to always round up > > instead > > of truncating down. This simplifies the code a bit but also introduces > > a bit > > more magic. > > > > To understand why the code ended up being simpler: > > First to round up you need to change this line > > "count >>= 1;" to this "count = (count + 1) >> 1;". > > Now let's move the "count--;" line before while cycle. > > To compensate you remove the "-1" in while condition and adjust the > > shift line > > like this: "count = ((count + 2) >> 1) - 1;". > > But now if you look at it it just does exactly the same thing as simple > > "count >>= 1;". > > Then just merge decrement and underflow prevention. > > > > It seems to be the right thing to do. When user requests some period we > > can't > > setup it's better to set longer period rather than shorter so that > > watchdog > > wouldn't timeout before it was supposed to do so. Well I guess most > > people > > won't care whether watchdog timeouts 10% sooner or later, let alone some > > rounding. > > > > After this it should be relatively easy to solve this issue: > > https://github.com/libopencm3/libopencm3/issues/533 > > Just by adding lines such as > > #ifndef LSI_FREQUENCY > > #define LSI_FREQUENCY 40000 > > #endif > > where needed. > > > > include/libopencm3/stm32/common/iwdg_common_all.h | 40 > > ++++++++++++++++++++++- > > lib/stm32/common/iwdg_common_all.c | 38 > > ++++++++++----------- > > 2 files changed, 57 insertions(+), 21 deletions(-) > > > > diff --git a/include/libopencm3/stm32/common/iwdg_common_all.h > > b/include/libopencm3/stm32/common/iwdg_common_all.h index > > d7430b53..996876d0 100644 > > --- a/include/libopencm3/stm32/common/iwdg_common_all.h > > +++ b/include/libopencm3/stm32/common/iwdg_common_all.h > > @@ -34,6 +34,18 @@ specific memorymap.h header before including this header > > file.*/ > > > > /**@{*/ > > > > +/** @addtogroup iwdg_file IWDG peripheral API > > +@ingroup peripheral_apis > > +@{ > > +*/ > > +#ifndef LSI_FREQUENCY > > +/** LSI freqency is 32 kHz on most chips. Define your own LSI_FREQUENCY > > before > > +including iwdg header file if your device's LSI is running on > > different clock. > > +*/ > > +#define LSI_FREQUENCY 32000 > > +#endif > > +/**@}*/ > > + > > /* --- IWDG registers > > ------------------------------------------------------ */ > > > > /** Key Register (IWDG_KR) */ > > @@ -104,13 +116,39 @@ specific memorymap.h header before including this > > header file.*/ > > BEGIN_DECLS > > > > void iwdg_start(void); > > -void iwdg_set_period_ms(uint32_t period); > > +void iwdg_set_period_ticks(uint32_t period); > > bool iwdg_reload_busy(void); > > bool iwdg_prescaler_busy(void); > > void iwdg_reset(void); > > > > END_DECLS > > > > +/** @addtogroup iwdg_file IWDG peripheral API > > +@ingroup peripheral_apis > > +@{ > > +*/ > > + > > +/*---------------------------------------------------------------------------*/ > > +/** @brief IWDG Set Period in Milliseconds > > + > > +The countdown period is converted to ticks of LSI clock. The > > maximum period is +1048576000 / LSI_FREQUENCY (32.768 seconds > > for 32 kHz LSI clock). For periods +less than 1ms you may want > > to use iwdg_set_period_ticks directly. > > + > > +Note that this library assumes 32 kHz LSI clock by default. If > > your device's +LSI runs on different clock define your own > > LSI_FREQUENCY before including iwdg +header file. > > + > > +@param[in] period uint32_t Period in milliseconds (<= > > 1048576000 / +LSI_FREQUENCY) from a watchdog reset until a > > system reset is issued. > > +*/ > > +static inline void iwdg_set_period_ms(uint32_t period) > > +{ > > + iwdg_set_period_ticks(period * LSI_FREQUENCY / 1000); > > +} > > + > > +/**@}*/ > > + > > #endif > > /** @cond */ > > #else > > diff --git a/lib/stm32/common/iwdg_common_all.c > > b/lib/stm32/common/iwdg_common_all.c index 44b3b9b3..6e597722 > > 100644 > > --- a/lib/stm32/common/iwdg_common_all.c > > +++ b/lib/stm32/common/iwdg_common_all.c > > @@ -39,7 +39,6 @@ relevant bit is not set, the IWDG timer must be enabled > > by software. > > > > #include <libopencm3/stm32/iwdg.h> > > > > -#define LSI_FREQUENCY 32000 > > #define COUNT_LENGTH 12 > > #define COUNT_MASK ((1 << COUNT_LENGTH)-1) > > > > @@ -57,45 +56,44 @@ void iwdg_start(void) > > } > > > > > > /*---------------------------------------------------------------------------*/ > > -/** @brief IWDG Set Period in Milliseconds > > +/** @brief IWDG Set Period in Ticks of LSI clock > > > > The countdown period is converted into count and prescale values. The > > maximum > > -period is 32.76 seconds; values above this are truncated. > > Periods less than 1ms -are not supported by this library. > > +period is 1048576 (0x100000) ticks; values above this are > > truncated. > > > > -A delay of up to 5 clock cycles of the LSI clock (about 156 > > microseconds) -can occasionally occur if the prescale or > > preload registers are currently busy -loading a previous value. > > +A delay of up to 5 clock cycles of the LSI clock can > > occasionally occur if the +prescale or preload registers are > > currently busy loading a previous value. > > > > -@param[in] period uint32_t Period in milliseconds (< 32760) > > from a watchdog -reset until a system reset is issued. +Note > > that even with lowest prescaler setting LSI clock is divided by > > 4 so you +can't set period shorter than 4 ticks. > > + > > +@param[in] period uint32_t Period in ticks of LSI clock (<= > > 1048576) from a +watchdog reset until a system reset is issued. > > This is rounded up to nearest +possible period. > > */ > > -void iwdg_set_period_ms(uint32_t period) > > +void iwdg_set_period_ticks(uint32_t period) > > { > > const int PRESCALER_MAX = 6; > > uint8_t prescale = 0; > > > > - /* Set the count to represent ticks of 8kHz clock (the 32kHz LSI clock > > - * divided by 4 = lowest prescaler setting) > > + /* Set the count to represent ticks divided by 4 (lowest prescaler > > + * setting) rounding up (ceil) the original period > > */ > > - uint32_t count = period << 3; > > + uint32_t count = (period + 3) >> 2; > > > > - /* Prevent underflow */ > > - if (count == 0) { > > - count = 1; > > + /* IWDG_RLR actually holds count - 1 (but prevent underflow) */ > > + if (count > 0) { > > + count--; > > } > > > > /* Shift count while increasing prescaler as many times as needed to > > * fit into IWDG_RLR > > */ > > - while ((count - 1) >> COUNT_LENGTH) { > > + while (count >> COUNT_LENGTH) { > > count >>= 1; > > prescale++; > > } > > > > - /* IWDG_RLR actually holds count - 1 */ > > - count--; > > - > > /* Clamp to max possible period */ > > if (prescale > PRESCALER_MAX) { > > count = COUNT_MASK; > > -- > > 2.11.0 > > _______________________________________________ libopencm3-devel mailing list libopencm3-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libopencm3-devel
