Finite Machine states inherit from OpInterface

[t0mpr1c3]

Restructuring the finite machine states to have a common set of methods including update() called from the main loop, and com() for serial communication. The Fsm object is renamed Controller.

Encoder values are saved at the top of Controller::update() instead of the ISR. This fixes a bug identified by @jpcornil-git. Now the same set of encoder values will be used even if an interrupt occurs during knit(). I tried to incorporate ATOMIC_BLOCK macro for this purpose, so that interrupts cannot occur during the saving of the values , but it requires includes from avr-libc and compilation using --std=c99. I couldn't get it to work, so it is currently commented out .

There is also a bugfix in the API (#172). The firmware expected reqTest to be a 2-byte message with the second byte specifying machine type, but the API specifies a 1-byte message, and that was what the desktop has been sending. (#AllYarnsAreBeautiful/ayab-desktop#461)

Another change, which I think is a bugfix (#173): after the knitting operation ends, it returns to the 'initialization' state (OpState_t::OpInit) rather than the 'ready to knit' state (OpState_t::OpReady).

Everything is mocked and test coverage is 100% except for the Knit operation. If the tests were working before then they probably still work now, but GCOVR seems unable to reach some of the branches .

To do list:

• try to get ATOMIC_BLOCK to compile
• move OpKnit::isReady() to OpInit where it belongs
• fix paths to OpTest

[jsarrett]

On an AVR I don't think you need to save SREG, the cli is enough. SREG is pushed on the stack as one of the interrupt dispatch delay cycles IIRC. You do want to do it though if there's a chance of it being called in a nested context. A common paradigm is to have a block of state that ISRs update, and cli(), copy to working set ,sei() at the top of your mainloop.
Regardless, ATOMIC_BLOCK() is way better than rolling your own, and should "just work" when compiled with g++ as in Arduino.

[jpcornil-git]

You do, only the program counter is pushed to the stack not SREG, see https://microchipdeveloper.com/8avr:int

The new implementation is doing the following:

• ISR updates a set of variables in its own context
• main loop call a method that enter a critical section to copy these variables to the main/local context

Note that a critical section is not cli()-critical code-sei() because if you do that you always enable interrupt at exit and this may not always be the desired behavior, i.e. you have to save the interrupt enable/disable state at entry and restore it at exit as illustrated below:

in r0, SREG     <- Save SREG in r0 (all flags including interrupt enable/disable status)
cli             <- Disable global interrupts
push r0         <- push r0/SREG to the stack (r0 may not be preserved by critical code below)

# critical code

pop r0          <- pull SREG from the stack
out SREG, r0    <- Restore SREG

All methods proposed in this thread aim to compile/expand into such code (macro with for loop using a "cleanup" attribute, macro leveraging constructor/destructor of a c++ class or just inline code). The issue with the ATOMIC_BLOCK() macro is that it relies on a "cleanup"/gcc-specific attribute to trigger the epilog (pop/out) code and is therefore less portable/supported by different compilers.

One thing we could add to further optimize the code (to save cycles) is only enter the critical section based on a new boolean flag set by the ISR (and reset it within the critical section)

[jsarrett]

@jpcornil-git right, but the ISR() macro, (and every sane interrupt handler ever written) will push SREG on the stack in it's function header. You're only worried when you call ATOMIC_BLOCK inside another ATOMIC_BLOCK that exiting the most recent one shouldn't automatically re-enable interrupts because they were still disabled by the prior block.

As for portability across compilers, You've already committed to the Arduino, so using the features of the only compiler (and libc!) available there isn't meaningfully affecting portability. It's also coming at the cost of maintainability and readability. Pretty much everyone can look up ATOMIC_BLOCK (which is well tested and maintained by the compiler writers, but not everyone can read and debug an assembly macro which is tied to the processor execution state.

If you wanted to remove all Arduino libraries/functions then it might make sense to not use avr-gcc intrinsics, but you'd have some other problem getting the other linker to locate the ISRs and properly exit them. There's no C/C++ standard for this, so no matter what you do it's going to be compiler specific.

[jsarrett]

Maybe that's a bit to harsh, I admit that there are very careful ISRs which are written in assembly and don't touch the SREG and can be a few cycles faster. But that's sort-of the point, if we're living at the C level of abstraction, then ATOMIC_BLOCK means we don't have to care. The team who bridged the abstraction level already cared for us and committed to keeping it working so we don't have to.

[jpcornil-git]

We are not talking about ISR but about a critical section in the main loop that is fetching data owned by an ISR.
You have to save the interrupt state before disabling it in order to restore it as it was (this code may be running before interrupts are enabled).

Note that ATOMIC_BLOCK(ATOMIC_RESTORESTATE) macro is doing just that as well:

see https://github.com/vancegroup-mirrors/avr-libc/blob/master/avr-libc/include/util/atomic.h#L205

static __inline__ void __iRestore(const  uint8_t *__s)
{
    SREG = *__s;
    __asm__ volatile ("" ::: "memory");
}
...
#define ATOMIC_BLOCK(type) for ( type, __ToDo = __iCliRetVal(); \
	                       __ToDo ; __ToDo = 0 )
...
#define ATOMIC_RESTORESTATE uint8_t sreg_save \
	__attribute__((__cleanup__(__iRestore))) = SREG

The initialisation part of the for loop is storing SREG in sreg_save and the __iRestore callback is restoring it (but rely on a non-standard compiler attribute)

Wrt Arduino, this is not 100% correct, platformIO is also used and will have to support esp32 moving forward
=> portability of the code across platforms is also important

[jpcornil-git]

Given that ATOMIC_BLOCK macro is used by other libraries we are already using, e.g. Serial, we should probably just reuse it here as well and adapt if/when we face a compiler issue, i.e.

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
//critical code
} 

[t0mpr1c3]

Given that ATOMIC_BLOCK macro is used by other libraries we are already using, e.g. Serial, we should probably just reuse it here as well and adapt if/when we face a compiler issue, i.e.

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
//critical code
} 

I agree. It's used by analogReadAsync as well.

[t0mpr1c3]

The compiler issues I had with ATOMIC_BLOCK were because I tried to enforce --std=c99. I stopped doing that and it compiled fine.

[jsarrett]

@jpcornil-git on a single core processor, with no caches, the only thing a "critical section" does to ensure safe memory access is disable interrupts. My point is that sei() and cli() are enough if all the ISRs save and restore the SREG when they return. The real benefit to saving SREG in non-interrupt context is that you can nest (i.e. call a function that also disables interrupts) the critical sections without worry. Plenty of systems out there just do cli()/sei() at the top of the mainloop to get fresh values to act upon, with complete memory safety. I'm not advocating for that style vs. critical sections all over the place.

I am advocating for doing all the platform specific stuff all in one place that's easy to swap out, and also avoiding writing/maintaining any platform specific stuff that's covered by the platform runtime.

[jpcornil-git]

Hi @jsarrett,

Imagine that you have a code setting up everything including interrupt handlers but with interrupt disabled to start with.
Then you start the mainloop including calls to pieces of code using critical sections but the application only enables interrupts after getting some configuration messages/events/...

If you enclose the critical section with a simple cli()/sei() then interrupts will be enabled straight away/before the application is configured/ready/... while if you save SREG.bit 7 (Global Interrupt Enable) before the critical section and restore it just after, interrupts stay disabled until enabled explicitely by the application later on, i.e. sei()/cli() is enough if interrupts are enabled/usable from the start but is not generic enough to cope will all use cases. and this is why ATOMIC_BLOC macro is saving/restoring SREG as well.

[jsarrett]

I mean, that's true, but a little contrived, and making my point for me. It seems like now we all agree that we should use the ATOMIC_BLOCK instead of reinventing it, since it handles SREG in an always safe way.

if you wanted to reinvent it, I think the point that you have to be careful about when/how you CLI/SEI stands, but that was my original complaint. If you're trying to optimize, you can do better (but you shouldn't).

For portability, I suggest that limiting the places where that happens will make life easier in the long run, and lead to a cleaner design.

[jpcornil-git]

Not sure why you say it is contrived (it is a rather generic pattern :-) but anyway, ATOMIC_BLOCK(ATOMIC_RESTORESTATE) addresses that and because it is also used in libraries we are using it should also be adopted here to avoid more than one protection scheme of critical code on a given platform.

In this application, single thread with ISRs, it should be used in the former wherever you pass/sync data to/from the latter.