search

pierremolinaro / acan2517FD

Distribution of Arduino driver for MCP2517FD CAN controller (CANFD mode)
MIT License
70 stars 17 forks source link

noInterrupts() and Interrupts() call potentially unneccesary #26

Open Flole998 opened 3 years ago

Flole998 commented 3 years ago

I'm currently looking into code similar to this which can be found all over the library:

https://github.com/pierremolinaro/acan2517FD/blob/6ee34ab8408225a1a126c1b2e49b0c67ad4c365a/src/ACAN2517FD.cpp#L726

As far as I can see, as we are using attachInterrupt, the mSPI.beginTransaction (mSPISettings) ; a few lines before is taking care of that:

If your program will perform SPI transactions within an interrupt, call this function to register the interrupt number or name with the SPI library. This allows SPI.beginTransaction() to prevent usage conflicts. Note that the interrupt specified in the call to usingInterrupt() will be disabled on a call to beginTransaction() and re-enabled in endTransaction().

Looking at the SPI Library that seems to be the case: https://github.com/arduino/ArduinoCore-avr/blob/24e6edd475c287cdafee0a4db2eb98927ce3cf58/libraries/SPI/src/SPI.h#L181 As we are using attachInterrupt() the interruptMode is greater than 0 so it should not be necessary to do this.

Unless someone has any reason to not do so I would suggest to remove the noInterrupts() and Interrupts() calls. As far as the taskDISABLE_INTERRUPTS () ; for the ESP is concerned I think this should be moved before the beginTransaction() to prevent a race condition there. An interrupt could occur after the beginTransaction() call or in the middle of it messing up the settings. Same thing for the enabling, that should be done after the endTransaction().

These changes should remove quite some instructions from each SPI transaction, thus increasing the performance.

Flole998 commented 3 years ago

After reading https://github.com/pierremolinaro/acan2517/issues/5 I am not sure if all of my suggestions would work: Apparently there is a reason why noInterrupts was added there. However, this is no longer needed on the MCP2518FD so we could add a #ifdef there and introduce a DISABLEMCP2517FDSUPPORT define which disables this workaround if it's not needed as the faulty 2517 is not used.