Improvements & integration issues

[bertmelis]

I've been digging a bit in esphome. As I understand, you have a setup and loop function for the integration and for each sensor/Datapoint you define.

So my approach would be to set a flag when reading or writing and in the loop function you try to act upon the flag and only clear when successful.

This way a queue isn't needed at all.

VitoWiFi at this moment doesn't return a value when making a request so this way of work isn't possible. It's a trivial change though.

[j0ta29]

@bertmelis , wouldn't that result in rejecting every read/write request for the following datapoints as long a there isn't a response for the first request? The maximum size of VitoWiFiClass::_queue would then be 1 and the current mechanism of buffering request in the phase of heavy load would get lost. Or did I misunderstand your proposal?

[bertmelis]

You're right for the most part.

Except: you don't clear the flag until the request was passed successfully to VitoWiFi. You just keep trying until a true is returned. The only catch is that you may never read/write the "last" datapoints if your request speed is faster than the answers come in.

[j0ta29]

Only to make sure I do not misunderstand your point: The new flag is a) something I will get in a coming version of VitoWiFi or b) something I have to implement by myself outside of VitoWiFi (set it when I call VitoWiFi.readDatapoint or VitoWifi.writeDatapoint and reset it when the result of the request arrives in the datapoint callback) c) ... ???

I have to admit, I do not really understand the term "until the request was passed successfully to VitoWiFi". Did you mean "until the request was responded successfully by VitoWiFi"?

The only catch is that you may never read/write the "last" datapoints if your request speed is faster than the answers come in.

Unfortunately this is the point, users currently struggle with. I think a queue (functioning as buffer) is still needed to smooth out peaks of request load in times where requests come faster than they can be processed. Either implemented in VitoWiFi or outside by the client of VitoWiFi. If I had a free wish: keep the queue within VitoWiFi and let the client specify the maximum size and allow the client to read the current load of the queue (just in case there is a plan b if the queue is full).

[bertmelis]

The thing with a queue is that it can always fill up if not properly configured. You can merely hide the underlying problem.

Conceptually I advocate for a mechanism like this:

1. esphome triggers a read by the scheduling system
2. the integration sets the read flag, the flag is a bool in the sensor component class
3. in the sensor component, you check the flag on each call of the loop function.
4. if the flag was set, try to read via optolink:
   • if read is successful (returns true), clear the flag
   • if read not successful, keep the flag and try again next time loop is called
5. when looping over other sensors, reading will fail because vitowifi returns false as it is busy. as soon as the first request finishes, the next request will succeed

Return a bool on read or write is something that can also be done in the current version of VitoWiFi. It's been several years since I've looked at the code and now I wonder why I didn't do it in the first place.

[j0ta29]

With with your description about the queue size in https://github.com/j0ta29/esphome/issues/4#issuecomment-1831744288 the penny dropped for me. At least I hope so ;-): The flag IS NOT a static class member that acts as a single gate for the VitoWiFi queue. Instead there is a flag for every instance of a sensor component (which correspond to a single datapoint) that prevents a datapoint from being queued if it's previous request is not yet processed. Is my assumption correct?

[bertmelis]

yes, a flag for each sensor.

of course, for a datapoint that is to be written, the value has to be copied to a member variable too.

[j0ta29]

Implemented in https://github.com/esphome/esphome/pull/4453/commits/7865a10d07b087a82f9de9e8f48a43a45d9c4381