Fix for SREG error - with this flow sensor is working

Post Reply
User avatar
RogerClark
Posts: 7683
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Fix for SREG error - with this flow sensor is working

Post by RogerClark » Mon Jul 03, 2017 7:01 am


victor_pv
Posts: 1864
Joined: Mon Apr 27, 2015 12:12 pm

Re: Fix for SREG error - with this flow sensor is working

Post by victor_pv » Mon Jul 03, 2017 11:22 pm

@Stevestrong, can you have a look at the SREG part and say what you think?
I think this could potentially be used in the latest SPI optimizations that enabled and disabled the interrupts.
In the code we discussed, you disabling interrupts before a time critical loop, then enable them again on exit. But what is the interrurps were disabled before the SPI function was called? It would exit with interrurpts enabled.

This SREG command I guess comes from the Arduino, but the function is to get the status of the interrupts before disabling them. If they were already disabled, then you dont enable them during the piece of code that needed them disabled.
It makes sense to me that we should include this class. Besides helping with compatibility with Arduino, we may need to use it in parts like that SPI code, so we know if interrupts need to be enabled or not.
This loop:
https://github.com/rogerclarkmelbourne/ ... I.cpp#L321

About the pintointerrupt part, I have no comment.

stevestrong
Posts: 2063
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Fix for SREG error - with this flow sensor is working

Post by stevestrong » Tue Jul 04, 2017 5:31 am

The sreg is part of a general issue: convert avr code to stm32.
The propsed solution is dangerous, although the author only uses it for sending data over can bus, i can imagine someone using it with spi, and then it will conflict with it, as Victor said.
In general, i recommend rewriting the code part in question. If avr compatibility is required, then use ifdef.
And i hardly believe that when using avr code, sreg is the only register that would need a fix.
Do we want to emulate all registers of the avr chips?
If yes, then we need a different approach.
If not, i do not agree with this fix.

The other story digitalpintointerrupt, i dont know what is the idea behind, so i cannot comment it either. But i think Roger applied already a fix for that.

victor_pv
Posts: 1864
Joined: Mon Apr 27, 2015 12:12 pm

Re: Fix for SREG error - with this flow sensor is working

Post by victor_pv » Tue Jul 04, 2017 6:27 am

stevestrong wrote:
Tue Jul 04, 2017 5:31 am
The sreg is part of a general issue: convert avr code to stm32.
The propsed solution is dangerous, although the author only uses it for sending data over can bus, i can imagine someone using it with spi, and then it will conflict with it, as Victor said.
In general, i recommend rewriting the code part in question. If avr compatibility is required, then use ifdef.
And i hardly believe that when using avr code, sreg is the only register that would need a fix.
Do we want to emulate all registers of the avr chips?
If yes, then we need a different approach.
If not, i do not agree with this fix.

The other story digitalpintointerrupt, i dont know what is the idea behind, so i cannot comment it either. But i think Roger applied already a fix for that.
Steve, my largest concern is that we actually need a mechanism like that to check if interrupts are enabled or not, otherwise there is code that will go and enable them while the user program did not expect that.
That is the current situation with the interrupts in the SPI library. The optimal look for sending and receiving requires disabling the interrups before each run of the loop, then enables then at the end of the run.
What if the user code had disabled interrupts for whatever reason and then calls that function to transfer a byte, and next interrupts are unexecpetedly enabled?
I do not know if emulating SREG is the best solution or not, but I thin we definitely need to add a function that reads primask before disabling and enabling global interrupts. I could not find any reference to the primask register in the core, so doesn't look like we have a function for that.
And the current implementation of SPI.transfer enables the interrupts without checking if they were actually enabled before the loop, so we need to modify that.

Teensy does use that SREG function, exactly the same, even though I found a thread with Paul saying that was not a soluation and so on, but at the end of the day they added that same thing to the core. So just for compatibility, I would add it like that.
We can add it with a different name and shape, but that will reduce compatibility with arduino and teensy, and anyway we need a funtion that pulls that mask.

stevestrong
Posts: 2063
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Fix for SREG error - with this flow sensor is working

Post by stevestrong » Tue Jul 04, 2017 7:33 am

Victor, I can understand you concern.

I see it this way: the IRQ disabling in the SPI code is for (speed-optimized) reading several bytes, not only one.
Disabling interrupts to read several bytes can lead to missing USB bytes and some systicks.
But still, some could find it useful/necessary, and in this case the user should know (or should be informed) that interrupts will be disabled and re-enabled when reading several bytes in non-DMA mode.

OTOH, the real story here is about SREG & co.
My opinion is that we don't need it.
Why is SREG so important? Only because of the interrupts? Then replace it with STM32 specific code, it can be posted on WIKI how to do it.
Is SREG the only register we should take care of when porting AVR code? I doubt.
And if there are several AVR regs to port, the user has to do anyway some more substantial change, so I think SREG does not need any special handling.

But this is only my opinion.

I will most probably never use it. I always save the STM32 sketches with a specific name so that it can be distinguished from AVR sketches.
And when it comes to port a whole AVR lib to SMT32, then I make anyway a separate lib out of it and remove all AVR specific stuff, or use ifdefs when the changes are not too many.

User avatar
RogerClark
Posts: 7683
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: Fix for SREG error - with this flow sensor is working

Post by RogerClark » Tue Jul 04, 2017 10:50 am

IMHO we should not attempt to emulate AVR registers, because if we do one, we have to do more and more e.g. all the GPIO

Best not to do it at all, and the user will need to port their code correctly to STM32

victor_pv
Posts: 1864
Joined: Mon Apr 27, 2015 12:12 pm

Re: Fix for SREG error - with this flow sensor is working

Post by victor_pv » Tue Jul 04, 2017 3:09 pm

RogerClark wrote:
Tue Jul 04, 2017 10:50 am
IMHO we should not attempt to emulate AVR registers, because if we do one, we have to do more and more e.g. all the GPIO

Best not to do it at all, and the user will need to port their code correctly to STM32
Ok, then lets add a function for checking if the interrupts are enabled, so for code like the SPI one we can check if they need enabling after a loop that needs them disabled.
stevestrong wrote:
Tue Jul 04, 2017 7:33 am
Victor, I can understand you concern.

I see it this way: the IRQ disabling in the SPI code is for (speed-optimized) reading several bytes, not only one.
Disabling interrupts to read several bytes can lead to missing USB bytes and some systicks.
But still, some could find it useful/necessary, and in this case the user should know (or should be informed) that interrupts will be disabled and re-enabled when reading several bytes in non-DMA mode.
I remember that conversation about the optimal SPI loop, I also came to the same conclusion we need to disable interrupts otherwise the RX DR could be overwritten and a byte lost. But most users won't know anything on the internals and will not expect interrupts to be enabled there just because.
I really think the right way to do it is check first if they are enabled. If they are, then enable them again after the critical part, if not, then leave them disabled.
I dont think is a good idea to modify a resource other than the SPI port and leave it modified in exit, when we can instead leave it the way it was.

About losing bytes from Serial or USB, there may not be enough time between enabling interrupts at the end of the loop and disabling them again.
According to ARM the cpu can execute up 3 instructions before servicing an interrupt. I dont remember the assembly, will need to find it in the thread, but could be as short as 1 instruction, just the branch, and then disable again.
For that they provide a special instruction that will trigger any ISR that was already pending:
http://infocenter.arm.com/help/index.js ... BFEIB.html

Did you have a piece of code that actually showed any lost bytes? if so, we can check if adding that resolves it.

stevestrong
Posts: 2063
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Fix for SREG error - with this flow sensor is working

Post by stevestrong » Tue Jul 04, 2017 4:21 pm

According to ARM info center, for M3/M4 CPUs it lasts up to 2 additional instructions to service a pending interrupt.
This is assured by the current code, discussed here.
I would agree to change the SPI code if problems come up.
If needed, lets further discuss the interrupts in other place. This should be directed to SREG.

Post Reply