Config timeout

[Miq1]

Some MODBUS slaves are really slow answering. For those it is handy to be able to config the timeout value at runtime.

[bertmelis]

good addition.

However: I didn't include the check for negative pin numbers because this is already done in the Arduino functions (pinMode and digitalWrite). Don't know what is better: check here and avoid the function call or check twice.

[Miq1]

I thought that it might be clearer with the explicit if around the digitalWrite() calls, since the behaviour on negative pin numbers seems not to be documented officially. So any port of the Arduino environment to another platform could break it. Performancewise it should not matter at all, as the call will have to load the pin number and check it against negative values anyway, so the compiler will most probably hold the comparison result in a register.

[Update] Uh, it is much worse... I checked the code in the AVR core source and found a disturbing implementation.

• The pin number is unconditionally converted into an uint8_t, giving a value of 255 for our int value -1

• Without any further checks, the resulting value is taken to consult a table in PROGMEM, that is much shorter than 255

• Finally you will end up with an arbitrary value taken from PROGMEM at offset 255 - whatever that is.

The ESP32 Arduino core does it differently, although converting the -1 int to an uint8_t as well, the check for validity involves a compare to 40. Everything larger is considered invalid.

So the unmodified code will work on an ESP32, but at least on "real" AVR Arduinos it will fail.

[bertmelis]

OK, I never looked to other platforms besides esp32/8266. And avoiding a function call is better then the redundant if.

Technically you can still assign 255. But I expect people to know what they're doing.

[bertmelis]

Why did you make the timeout static?

[Miq1]

Why did you make the timeout static?

Since I wanted to have just one class-wide value. If you are to deal with several esp32ModbusRTU instances at the same time, you may be better off with an object-based value, right.

Should I change it in another pull request?

Oh, by the way: I so far had created a separate branch for every change - should I stay with that or would you prefer larger chunks of changes for different topics?

[bertmelis]

I find it better to have it as a regular class member variable.

Oh, by the way: I so far had created a separate branch for every change - should I stay with that or would you prefer larger chunks of changes for different topics? Separate branches for separate PRs. I'd like to smash merge

[Miq1]

Okay, did so already (both: class member variable as another (well, 3 in fact) commit and separate branches ;))