simplefoc / Arduino-FOC-drivers

Drivers and support code for SimpleFOC
https://docs.simplefoc.com/drivers_library
MIT License
146 stars 63 forks source link

Minor modification request for AS5048A to be thread-safe on ESP32 #43

Closed sylque closed 4 months ago

sylque commented 4 months ago

This code from AS5048A.cpp is not therad-safe on ESP32:

uint16_t AS5048A::spi_transfer16(uint16_t outdata) {
    if (nCS>=0)
        digitalWrite(nCS, 0);
    spi->beginTransaction(settings);
    ...
    spi->endTransaction();
    if (nCS>=0)
        digitalWrite(nCS, 1);
    ...
}

It causes issues if the AS5048A is on the same SPI bus as another device, and both are accessed from separate tasks.

The reason is that the Arduino ESP32 SPI library implements thread safety in beginTransaction() and endTransaction(). So digitalWrite(nCS) should be placed inside those two in order to benefit from the mutex.

There's an example in MagneticSensorSPI.cpp, which doesn't have the issue:

word MagneticSensorSPI::read(word angle_register){
  ...
  //SPI - begin transaction
  spi->beginTransaction(settings);

  //Send the command
  digitalWrite(chip_select_pin, LOW);
  ...
  digitalWrite(chip_select_pin, HIGH);

  //SPI - end transaction
  spi->endTransaction();
  ...
}
runger1101001 commented 4 months ago

Hey, thanks for reporting it, and yes, it should be changed.

I'm not sure it is the correct approach in terms of your particular problem regarding thread safety, but if it works for you, then so much the better.

Its also necessary to change it just from the point of view of correctness, although the examples and documentation from Arduino isn't very clear, I believe the current version also leads to incorrect SPI behaviour in absence of any multi-threading.

It will be fixed for the next release.

runger1101001 commented 4 months ago

These changes are now on the dev branch of the library. Please let me know if you have a chance to test it on esp32...

sylque commented 4 months ago

Thanks @runger1101001, I confirm the dev branch works well on ESP32.