Skip to content

FAILEDREADFREQUENCY is overwritten #73

@lucasnz

Description

@lucasnz

In updateStatus(), if a read error occurs the function sets:
_nextUpdateDue = millis() + FAILEDREADFREQUENCY;

Reads (almost??) always occur after a sendCommand(). This function sets:
_resultRegistersDirty = true;

This means that _nextUpdateDue gets overwritten, in loop():
if (_resultRegistersDirty) {
_nextUpdateDue = millis() + 200; // if we need to read the registers, pause a bit to see if there are more commands coming.
_resultRegistersDirty = false;
}

This can result in repeated update requests being sent to the spa in the following scenarios:

  1. TX or RX wires aren't connected properly.
  2. There is an error in the code parsing the spa response.

Skip's Discord comment on the issue:
So the 200ms is a general delay, the idea being is that if there are a number of writes going to the spa then we don't want/need to be doing a read after each, you can send the writes and then do a single read.

In the original implementation I set up a fifo queue and only did a read when the queue was empty, I did away with this as it added complexity that I didn’t think that we needed

Failed read frequency is meant to be the time between failed RF commands, if we don't get a good result when parsing the RF array then we try again in 1 sec.

In hindsight my design is flawed as it assumes linear processing but there are non-linear elements to the code that may introduce “wriggles” such as the callback from a MQTT subscription.

Possible a duplicate of #51

Do we need to rethink (and reintroduce) the FIFO queue idea? Do we need to add code to limit the number of times we retry with a 200ms delay and then start backing off?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions