Things that should already be fixed in ConfigurableFirmata

[jcvillegasfernandez]

Bugs reported long ago and not fixed yet:

Fix the compatibility problem of FirmataScheduler and OnewireFirmata with devices other than Arduino Uno, like ESP8266, ESP32 and probably others due to different sizes of pointer, Int and long.

New bug detected:

A bug (hard to detect) when the size of the SYSEX command to send is 64 (buffer size), the last byte is lost, works fine with any other value, solution at the end.

Modify file FirmataScheduler.cpp in this way

inside this function boolean FirmataScheduler::handleSysex(byte command, byte argc, byte* argv)

case DELAY_FIRMATA_TASK:
          {
            if (argc == 6) {
              argv++;
              Encoder7BitClass::readBinary(4, argv, argv); //decode inplace
     **// long is different on ESP8266 (or ESP32) and arduino**
              _delayTask((long)argv[0] | (long)argv[1]<<8 | (long)argv[2]<<16 | (long)argv[3]<<24);
            }
            break;
          }
        case SCHEDULE_FIRMATA_TASK:
          {
            if (argc == 7) { //one byte taskid, 5 bytes to encode 4 bytes of long
              Encoder7BitClass::readBinary(4, argv + 2, argv + 2); //decode inplace
    **// long is different on ESP8266 (or ESP32) and arduino**
              _schedule(argv[1], (long)argv[2] | (long)argv[3]<<8 | (long)argv[4]<<16 | (long)argv[5]<<24);
            }
            break;
          }

void FirmataScheduler::reportTask(byte id, firmata_task* task, boolean error)

if (task) {
        encoder.startBinaryWrite();
    **// because long and int are different on ESP8266 (or ESP32) and arduino**
            // write time_ms
        encoder.writeBinary(task->time_ms & 0xFF); 
        encoder.writeBinary((task->time_ms >> 8) & 0xFF);  
        encoder.writeBinary((task->time_ms >> 16) & 0xFF); 
        encoder.writeBinary((task->time_ms >> 24) & 0xFF); 
           // write len
        encoder.writeBinary(task->len & 0xFF); 
        encoder.writeBinary((task->len >> 8) & 0xFF);
            // write pos
        encoder.writeBinary(task->pos & 0xFF); 
        encoder.writeBinary((task->pos >> 8) & 0xFF); 
            // write messages
        for (unsigned int i = 0; i < task->len; i++) {
          encoder.writeBinary(task->messages[i]); 
        }        
    encoder.endBinaryWrite();
    }

Modify file OnwwireFirmata.cpp in this way

inside this function boolean OneWireFirmata::handleSysex(byte command, byte argc, byte* argv)

 if (subcommand & ONEWIRE_READ_REQUEST_BIT) {
                  if (numBytes < 4) break;
                  //numReadBytes = *((int*)argv);
                  //correlationId = *((int*)argv);
   **// int is different on ESP8266 (or ESP32) and arduino**
                  numReadBytes = (int)argv[0] | ((int)argv[1]<<8);  
                  correlationId = (int)argv[2] | ((int)argv[3]<<8); 
		  argv += 4;
                  numBytes -= 4;
                }
  if (subcommand & ONEWIRE_DELAY_REQUEST_BIT) {
                  if (numBytes < 4) break;
                  //Firmata.delayTask(*((long*)argv));
   **// long is different on ESP8266 (or ESP32) and arduino**
                  Firmata.delayTask((long)argv[0] | ((long)argv[1] << 8) | ((long)argv[2] << 16) | ((long)argv[3] << 24));
                  argv += 4;
                  numBytes -= 4;
                }

Change ConfigurableFirmata.cpp (lost last byte when command size is 64)

Change inside this function

void FirmataClass::parse(byte inputData)

else {
       //normal data byte - add to buffer
       storedInputData[sysexBytesRead] = inputData;
       sysexBytesRead++;
if (sysexBytesRead == MAX_DATA_BYTES)
{
Firmata.sendString(F("Discarding input message, out of buffer"));
parsingSysex = false;
sysexBytesRead = 0;
 waitForData = 0;
       }
  }

For this

else {
if (sysexBytesRead == MAX_DATA_BYTES)
{
Firmata.sendString(F("Discarding input message, out of buffer"));
parsingSysex = false;
sysexBytesRead = 0;
           waitForData = 0;
         }
else {
           //normal data byte - add to buffer
           storedInputData[sysexBytesRead] = inputData;
           sysexBytesRead++;
         }
     }

[pgrawehr]

Thanks for reporting. Can you please open a pull request with your suggested changes?

I have not found an use case for the FirmataScheduler, can you provide information about how you use it and with which client library?

[echavet]

a better modification for compatibility should be to replace in

boolean OneWireFirmata::handleSysex(byte command, byte argc, byte* argv) {

numReadBytes = *((int*)argv);
argv += 2;
correlationId = *((int*)argv);
argv += 2;
numBytes -= 4;

by

numReadBytes = *((int16_t*)argv);
argv += sizeof(int16_t);
correlationId = *((int16_t*)argv);
argv += sizeof(int16_t);
numBytes -= 2 * sizeof(int16_t);

[pgrawehr]

@echavet Ok, that makes sense. I have never used the onewire module myself, so I cannot test this. Could you create a PR, please?

[awong1900]

On esp32, Encoder7BitClass::readBinary work not as expected. Such as:

    byte data[2] = {0x00, 0x00};
    byte test[4] = {0x01, 0x00, 0x01, 0x00};
    Encoder7BitClass::readBinary(2, test, data);

It shoud be get data is 0x01, 0x01, but now is 0x01, 0x04.

[pgrawehr]

@awong1900 That method works just fine, but maybe differently than what you would expect. This is not the reverse of sendValueAsTwo7bitBytes, but unpacks binary data from a densely packed bit stream. To pack the data, look at the writeBinary method or the reference implementation at https://github.com/dotnet/iot/blob/main/src/devices/Arduino/Encoder7Bit.cs.

The correct result for your example is 0x01, 0x40, 0x0 (3 bytes).

[echavet]

I didn't see the "Firmata.delayTask(((long)argv));" fix proposed.

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);
  }
}

What do you think of that ?

[pgrawehr]

@echavet Sorry, I've lost track of this issue. But are you saying that the fix in your last post is the only thing required to fix the FirmataScheduler problem?

Also, what client library are you using to test this?

[echavet]

I'm using johnny-five client library. There are bugs in the client one wire implementation in this library too. I have proposed corrections https://github.com/rwaldron/johnny-five/pull/1824 to allow the thermometer hardware to process with real delays.

What I'm saying is that there might be some misconceptions in the one-wire configurableFirmata, links to these delays.

I.m not sure it solved all the problems but I've managed to get it work with 3 or 4 thermometers connected to an Uno board which is connected via usb to a RPI 4. This works with homeassistant in an add-on I'm working on ( https://github.com/echavet/j5_ha_bridge).

[pgrawehr]

I don't have any one-wire devices, so I cannot test that implementation.

When you got it to work, did you need any changes to ConfigurableFirmata?