#136 : sizeOf Int depends on the board, use of int16_t OneWireFirmata…

[echavet]

…::handleSysex

Several users reports memory pb with the OneWire handleSysex method. I've tested this change on arduino Zero. No more freeze.

[pgrawehr]

Looking good, thanks. I do not know why the build is currently failing. This is certainly not related to the changes.

[pgrawehr]

The problem is the arduino-esp32 core, which breaks the build in v 2.0.8. Should hopefully be fixed soonish. See espressif/arduino-esp32#8108

[pgrawehr]

Note: Arduino-esp32 core 2.0.8 is not supported. Use 2.0.9 or later.

[echavet]

There remains an issue, potentially related to the one resolved here, for which jcvillegasfernandez has proposed a solution. The suggested fix was to replace this line:

Firmata.delayTask(((long)argv));

with this:

Firmata.delayTask(argv[0] | ((long)argv[1] << 8) | ((long)argv[2] << 16) | ((long)argv[3] << 24));

This is a bit surprising, given that all the platforms (esp8266, ardiono uno or zero for me) in question are little endian and the "long" data type is consistently 32 bits in size. Despite this, users have reported that the proposed code change resolves the issue!

For me, this problem arises when working with Arduino Uno, but not with Arduino Zero. Interestingly, the occurrence of the error seems to be contingent upon the inclusion of FirmataScheduler, as defined by the configuration typedef constant:

#ifdef ENABLE_BASIC_SCHEDULER

Looking for an explanation, I discovered that the implementation of the delayTaskCallback function is specific to FirmataScheduler. It calls Firmata.attachDelayTask(delayTaskCallback); for its software delay implementation:

void FirmataScheduler::delayTask(long delay_ms)
{
  if (running) {
    long now = millis();
    running->time_ms += delay_ms;
    if (running->time_ms < now) { // If delay time already passed, schedule to 'now'.
      running->time_ms = now;
    }
  }
}

I am no longer able to test this, but I believe that without ENABLE_BASIC_SCHEDULER, there is NO implementation for delayTaskCallback. Therefore, the delay operation is solely dependent on the runtime execution of the delayTask method:

if (delayTaskCallback) {
  (*delayTaskCallback)(delay);
}

Now let's consider the comparison between these two scenarios:

1. Three left shifts and three long casts: argv[0] | ((long)argv[1] << 8) | ((long)argv[2] << 16) | ((long)argv[3] << 24)
2. Versus a single long cast: Firmata.delayTask(*((long*)argv));

Given that I'm using the Johnny-Five library, and its implementation of the DS18B20 thermometer uses board.io.sendOneWireDelay(pin, 1); to allow for temperature conversion, it's plausible that this delay is primarily necessary for slower platforms.

My hypothesis is that there would be no discernible difference with a longer delay period when delayTaskCallback is null because there is no delay instruction.

/**
 * Call the delayTask callback function when using FirmataScheduler. Must first attach a callback
 * using attachDelayTask.
 * @see FirmataScheduler
 * @param delay The amount of time to delay in milliseconds.
 */
void FirmataClass::delayTask(long delayMs) {
  if (delayTaskCallback) {
    (*delayTaskCallback)(delayMs);
  } 
  // adding a default implementation seems to solve the issue
   else {
    delay(delayMs);
  }
}

---