About the use of MillisecondTimer for timeout

[tokoro10g]

Hi, I have a suggestion for the implementation of timeout.

Your code seem to have implemented it by using MillisecondTimer, but this fact is not well-documented.

I think that MillisecondTimer and the timeout functionality should be separated.
It is not trivial that timeout is automatically enabled when using MillisecondTimer so they should be switched like this:

(When using timeout)

I2C1_Default<I2CTimeoutFeature,...> i2c; // Add timeout feature which initialises MillisecondTimer automatically
// MillisecondTimer::initialise(); // Now this isn't necessary because of the timeout feature

(When using MillisecondTimer with no timeout)

I2C1_Default<SomeOtherFeature,...> i2c; // Doesn't use MillisecondTimer for timeout
MillisecondTimer::initialise(); // Necessary to use MillisecondTimer

Thank you for your fascinating library and I hopefully make it more efficient and useful.

[andysworkshop]

I'll look into this. There are other classes that use MillisecondTimer for timeout purposes and any approach I use should be consistent for all classes.

[mikepurvis]

Having broad dependence on MillisecondTimer is a problem for situations where you've got an RTOS or something which is wrapping SysTick. The ideal would be a Strategy Pattern, where you could optionally pass a reference at initialization time to an alternative backing implementation.

[andysworkshop]

Good point about the RTOS. I have an idea forming in the back of my ahead about how this could be done whilst still retaining compatibility.

[mikepurvis]

Hey, did anything come of this? I'm running into the conflict now trying to use the I2C peripheral.

[andysworkshop]

This change is on the mikepurvis-tick-event branch. I will merge it if I get positive feedback here

I've taken Mike's PR and extended it to abstract MillisecondTimer from being a concrete class to being a type that you can replace at compile time with a custom implementation. Hopefully this will help the library to co-exist with other frameworks that want to implement the SysTick interrupt themselves.

This is a non-breaking change. Existing code will work just like it did before. If you want to wire up the stm32plus millisecond timer into another frameworks implementation then you can:

Define CUSTOM_MILLISECOND_TIMER in your project's build. This alone will cause MillisecondTimer to be undefined.

You should provide your own implementation class that satisfies the 3-method contract defined in MillisecondTimerHolder.

Here's an example that replaces the simple default with Mike's idea of permitting an event-based millisecond callback:

#define CUSTOM_MILLISECOND_TIMER

#include "config/stm32plus.h"
#include "config/timing.h"

namespace stm32plus {
  typedef MillisecondTimerHolder<SubscribableSysTickMillisecondTimer> MillisecondTimer;
}

This implementation of MillisecondTimer allows subscriptions:

MillisecondTimer::MillisecondTickEventSender.insertSubscriber(MillisecondTickEventSourceSlot::bind(this,&MyClass::onTick));

and the callback is a simple void method

void MyClass::onTick() {
}

[tokoro10g]

+1
The functionality of MillisecondTimer is for general purpose and I think this change will not break its simplicity.

[mikepurvis]

Two thoughts:

First, it's a bummer to have to have a library-compile-time flag around this. It's one more variant of the library for the purposes of generating distributable static libs. I don't know much about how ISRs get linked up and so-on, but is it possible that the ISR could be defined in a header, so that the decision whether to use it or not can be made at project compile time rather than library compile time? Maybe there's no way to do this without having a separate timer ISR header which has to be included only once across all sources, or a #define STM32PLUS_SYSTICK_ISR that you have put in only one source to have it created and avoid a conflict.

Second, it seems to me that an event like MillisecondTick should be available regardless of what your backing MillisecondTimer implementation is— the implementation supplies ticks, whereas the rest of the library are consumers of ticks. Whether I want to attach code to the systick should be independent of what's providing it.

Thoughts?

[mikepurvis]

It's not as portable, but declaring the supplied SysTick ISR as a weak symbol might address both issues— it would allow the linker to throw away multiple instances of the ISR coming in via headers, but it would also permit a user-supplied instance of the ISR to override the library-supplied one.

[andysworkshop]

The issue with using weak symbols for the ISRs is that weak definitions already exist in the Startup.asm file that direct unused ISRs to the WWDG_IRQHandler entry point. If the library ISRs were also weak then the linker rule is 'choose any' ... not good.

[mikepurvis]

My principal concern is what to do about a precompiled static lib. If we go with SubscribableSysTickMillisecondTimer, then I'll probably patch the launchpad release so that that's the default (in large part, obviously, because my application needs it, and I'm not especially space-sensitive).

What about just making the default handler supplied by stm32plus a weak symbol, and eliminating the weak alias to Default_Handler from the Statup.asm files in the examples? Anyone starting a new project is likely to use one of the examples, so the primary risk at that point would be that someone might upgrade 3.4 -> 3.5 and not realise that the default handler was now in jeopardy. That's a matter for noting in the release documentation.

A more dramatic case might be made that as a general purpose library, stm32plus should avoid all non-weak ISRs, and that any of them provided by the library should be opt-in by users, either using their project's Startup.asm file, or some other mechanism.

[andysworkshop]

What about just making the default handler supplied by stm32plus a weak symbol, and eliminating the weak alias to Default_Handler from the Statup.asm files in the examples

This is a good simple solution and I'm in favour of it. I've tried it on a few of the examples and it does work well. The issue with upgraders is, I think, minor and can be addressed in release notes as you suggest. I'll prepare a commit shortly with all the startup.asm files adjusted.

A more dramatic case might be made that as a general purpose library, stm32plus should avoid all non-weak ISRs, and that any of them provided by the library should be opt-in by users, either using their project's Startup.asm file, or some other mechanism.

I'm actually in favour of this as well. Currently there are #ifdef's that guard the other ISRs but that's a clunky solution requiring a library rebuild to opt out. The weak symbol method is much better in my opinion. It's a bit too much for 3.5.0 though so I'll save it for the next release after that.

There will be a 3.5.0 release in fairly short order. I'm just in the process of overhauling the physical layer of the TCP/IP stack (in a non-breaking way) and would like to get those changes into 3.5.0

[andysworkshop]

This is now on master as commit 08aef0f. Existing projects that try to build against this release that don't remove the weak aliasing of SysTick_Handler to the dreaded WWDG_IRQHandler from startup.asm will most likely fail. This is a potential pain, but also an easy fix and there seems to be so much interest in the resolution of this issue that I think it's worth it.

[andysworkshop]

With the release of 3.5.0 I hope no-one minds if I close this now.

[mikepurvis]

Waking this thread up again. Sorry I was kind of lukewarm on the MillisecondTimerHolder idea at the time, but having delay be a swappable affair would also be really valuable, for the case where you want to yield your RTOS thread when blocking on a peripheral.