Definition of variable startTime in _readResponse

[bouncing-ball]

Hi,

Thanks for sharing this nice and well documented library.

I have a question re. the _readResponse function. In there I see that variable startTime is being used to store/compare millis() as well as micro(s). Isn't that confusing, and possibly also a bug? Wouldn't it be better to have startTimeMillis and startTimeMicros ?

[CMB27]

Yeah, that might be confusing, but it is not a bug.

At the beginning of the function, startTime is used to determine if a response timeout condition has occurred. The _responseTimeout is set in microseconds.

But if _serial->available() returns true, the variable is reutilized to track the timing of receiving the response. All these measurements are in microseconds. startTimeout is never used again to track milliseconds; the function returns.

[bouncing-ball]

OK, I think I see what you mean, thank you. The "while" condition (in millis) at the beginning of the function is only evaluated once, as further down in the code of that function, every thing ends with a "return". So the "while" is more like an "if" here, correct?

[CMB27]

More or less. But prompted by your comment, I did rewrite that function to be more clear.

uint16_t ModbusRTUMaster::_readResponse(uint8_t id, uint8_t functionCode) {
  uint32_t startTime = millis();
  uint16_t numBytes = 0;
  while (!_serial->available()) {
    if (millis() - startTime >= _responseTimeout) {
      _timeoutFlag = true;
      return 0;
    }
  }
  do {
    if (_serial->available()) {
      startTime = micros();
      _buf[numBytes] = _serial->read();
      numBytes++;
    }
  } while (micros() - startTime <= _charTimeout && numBytes < MODBUS_RTU_MASTER_BUF_SIZE);
  while (micros() - startTime < _frameTimeout);
  if (_serial->available() || _buf[0] != id || (_buf[1] != functionCode && _buf[1] != (functionCode + 128)) || _crc(numBytes - 2) != _bytesToWord(_buf[numBytes - 1], _buf[numBytes - 2])) return 0;
  else if (_buf[1] == (functionCode + 128)) {
    _exceptionResponse = _buf[2];
    return 0;
  }
  return (numBytes - 2);
}

I am still reusing the variable startTime but it should be easier to see where it is used for milliseconds and where it is used for microseconds.