ESP8266 and ExternalInterruptHandler

[littleyoda]

Hi

testing my framework for a esp8266, I found three problems in ExternalInterruptHandler.
Because I'm not sure how to fix the problems platform independent, I have not created a Pull Request.

1. All Functions called by an interrupt must be declared as ICACHE_RAM_ATTR

=>
ICACHE_RAM_ATTR void ExternalInterruptHandler(void)

2. millis()
   millis() return a unsigned long on esp8226 and esp32. lastMicros, actMicros and bitMicros are currently declared as word/unsigned int. For the esp8266 and esp32 unsigend long is necessary,

static word lastMicros;
unsigned int actMicros, bitMicros;

=>

static unsigned long lastMicros = 0;
unsigned long actMicros, bitMicros;

[MicroBahner]

Hi Sven,

All Function called by an interrupt must be declared as ICACHE_RAM_ATTR

Is that different from ESP32, or can we use the same attribute as in ESP32?

millis()
millis() return a unsigned long on esp8226 and esp32. lastMicros, actMicros and bitMicros are currently declared as word/unsigned int. For the esp8266 and esp32 unsigend long is necessary,

micros() returns always unsigned long - even on AVR processors. Because we only need to measure times less then 65000µs, I decided to use unsigend int, because this is faster an an 8-bit processor like ATmega.
On processors with a 32bit Core ( e.g. esp8266 ) 'unsigned int' and 'unsigend long' ist the same.
'unsigend long isn't neccessary, With 'unsingend int' the code is platform independent.

But you are right with the 'static word lastMicros'. To be independent how 'word' is defined, we should use 'unsigned int' there too.
=> static unsigned int lastMicros = 0;

Edit: 'word' seems to be defined as 'uint16_t' on esp and as 'unsigned int' on STM32, which of course is incompatible. On an AVR both is the same. So we should not use 'word' in any place.

[MicroBahner]

Hi,
When we introduced the support for ESP8266 it worked, so what had changed?
I tried with an older version (2.0.0) of the esp8266 Software, and in this version 'word' came out to be a 32bit variable.
Obviously the definition of 'word' has been changed in one of the following versions.
We must not use 'word' in variable definitions and change that to 'unsigend int'. ( That was my mistake :( )

[kiwi64ajs]

Hi Guys, I’ve made tweaks to the code as suggested by Sven and pushed the changes onto the ESP8266-ICACHE_RAM_ATTR branch here: https://github.com/mrrwa/NmraDcc/tree/ESP8266-ICACHE_RAM_ATTR <https://github.com/mrrwa/NmraDcc/tree/ESP8266-ICACHE_RAM_ATTR> 1) As far as ESP8266 vs. ESP32 I think because the ESP8266 only has a single core it doesn’t need the mutex that the ESP32 has, so I’ve just added an extra conditional compilation to add the ICACHE_RAM_ATTR <https://github.com/mrrwa/NmraDcc/tree/ESP8266-ICACHE_RAM_ATTR> to the ExternalInterruptHandler definition 2) I’ve change the storage for the Micros() value unsigned long as per the Arduino micros() documentation. Sven: - Was there anything else? - Can you test the code on the new branch to make sure the changes are correct? Regards Alex Shepherd

…

On 24/05/2019, at 12:06 AM, Franz-Peter ***@***.***> wrote: Hi, When we introduced the support for ESP8266 it worked, so what had changed? I tried with an older version (2.0.0) of the esp8266 Software, and in this version 'word' came out to be a 32bit variable. Obviously the definition of 'word' has been changed in one of the following versions. We must not use 'word' in variable definitions and change that to 'unsigend int'. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#25?email_source=notifications&email_token=AB5Y53NG4KOBVIM5AZK47HTPW2CC5A5CNFSM4HOXQ2ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWCADWY#issuecomment-495190491>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5Y53NNBNP4VZNGMCBOLQLPW2CC5ANCNFSM4HOXQ2ZA>.

[littleyoda]

Great. Thanks. I will test it.

@MicroBahner:
I have tested it with the old arduino framework quite extensive (=using it for my own turnout motors). Some weeks ago I received bug reports that the DCC Part does not work. I spend quite a while to debug this topic and found the problem with the millis(). Your observations explain this problem pretty well. Thanks

EDIT: Meanwhile I found the related Issue

Later crashes were reported. Results from the missing ICACHE_RAM_ATTR.
At the time I added the esp8266 support to NmraDcc, I was not aware of ICACHE_RAM_ATTR and I had no crashes in my application.

[kiwi64ajs]

AFAICT the need for the ICACHE_RAM_ATTR attribute really becomes an issue if you’re also using the SPIFFS, which is common with using a web server. If you get an interrupt while doing a SPIFFS operation you’ll get a crash. This behavior has changed a bit over time with different handling policies as Espressif responded to issues. HTH Regards Alex Shepherd

…

On 25/05/2019, at 6:29 PM, Sven ***@***.***> wrote: ICACHE_RAM_ATTR

[MicroBahner]

Hi Alex,

2. I’ve change the storage for the Micros() value unsigned long as per the Arduino micros() documentation.

I think we should stay to the solution with unsigned int. On 32bit Cores this is the same as unsigned long. But the AVR is a 8 bit processor, and it's faster with unsigned int than with unsigned long. Unsigned int is sufficient for our needs, it worked pretty well so far.

In an actual project I found another issue. When writing the IRQ without use of timer 0, I allowed nested interrupts, to give short, but timecritical interrupts a chance. Now the problem is the other way round: There is another comparabel long IRQ ( about 50...100µs) which also allows nested interrupts. But if such an IRQ interrupts the DCC IRQ, I run into trouble. So I need a possibility to switch of the nested IRQ feature.
My proposal is to make this feature configurable by a #define in the .h file. If you have timecritical short IRQ's you can enable it, but usually it should be disabled, bacause this is the more safe mode for DCC. It is easy, because you have only to mask out the interrupt-enable statement in the DCC-IRQ by this define.

I'll make a proposal (pull request) the next days after my tests.

Regards
Franz-Peter

[kiwi64ajs]

Ok, I believe I've merged in STM32 Nested Interrupts handling changes as well as the lastMicros type definition into master.

Before I tag the repo to 2.0.2 can you guys check that this doesn't break the STM32, ESP8266 and ESP32 versions as well as the original AVR - that would be very helpful as we've added a bunch of changes since 2.0.0 and it would be good to get them released.

[MicroBahner]

Hi Alex,

I did a first test and recognized that I am no longer able to read CV Values with my Märklin MS2.

I saw in servicemode there is a new callback 'notifyAdvancedCVAck' which is called instead of the old 'notifyCVAck', but only if the RailCom bit in CV29 is set. If this bit is not set, nothing is called at all. I am no RailCom expert, but I cannot imagine that this callback is sufficent to build a RailCom capable decoder. Acknowledgement and feedback in RailCom is completely different from the simple 6ms current pulse in standard decoders.

But in any case, I think if the railcom bit is NOT set, servicemode schould behave just like it was in earlier versions ( and call 'notifyCVAck' if a pulse has to be generated ).

Regards

Franz-Peter

I'll do further tests the next days/weeks ;)

[MicroBahner]

Hi Alex,
the last days I did intensive tests with ESP8266, STM32F103 and AVR. I tested together with my MobaTools what created heavy interrupt load. All worked well.
But I had to roll back the changes regarding 'notifyAdvancedCVAck', because reading of CV values did not work anymore. I'll create a pull request with these changes.

[kiwi64ajs]

See my comments in the other Issue. I've separated the types of Ack so the common code gets passed the function pointer for the correct Ack Function, so this should now work correctly for Service Mode again.