ratgdo / mqtt-ratgdo

ratgdo via mqtt
GNU General Public License v2.0
85 stars 20 forks source link

Possibly incorrect delay for secplus1 in toggleDoor() #15

Closed p-jean closed 11 months ago

p-jean commented 12 months ago

Found while reading the code in ratgdo.cpp:

void toggleDoor(){
    if(controlProtocol == "drycontact"){
        pullLow();
    }else if(controlProtocol == "secplus1"){
        uint8_t delayLen = (lastRX + 275) - millis();
        delay(delayLen);

The actual value of delayLen may not be as intended: 1) Due to the uint8_t truncation, the max delay will be 255. If the value of expression exceeds 255 it will wrap around to a very short delay. 2) If the expression resolves to a negative value (when lastRx was long ago) then the actual delay will be 0..255 when it probably should be 0 (I assume).

I don’t know enough to predict whether this causes any undesirable outcome. Curious: Is it correct that this version of RATGDO (secplus1) “hears itself” and so all TX is ultimately RX-ed?

See ratdgo.cpp:714 (commit 45b9263), at the beginning of the toggleDoor() function.

PaulWieland commented 11 months ago

Thanks for that! Will fix in next release.

I should mention that the delay should always be shorter than 255, but it's still not wise to use a uint8 instead of uint16.

PaulWieland commented 11 months ago

Corrected