Cereal2nd / velbus-aio

Velbus Asyncio
Apache License 2.0
15 stars 10 forks source link

Temperature flip-flops between low/high precision data #50

Closed niobos closed 1 year ago

niobos commented 1 year ago

Hi,

I discovered some behavior of velbus-aio that I would like to change. I'm willing to fix this in a PR, but since the current behavior is technically correct, this isn't a simple bug and needs discussing first. Hence this issue.

A temperature sensor (VMB1TS, VMBGP*) can report its current temperature in two forms:

Currently, the last received message is used to update the state of the temperature channel.

When graphing the temperature data, this causes "glitches" in the high-precision graph when a low-precision message is received: Note the sudden drop to 19.0°C around 9:30

Screenshot 2022-10-24 at 10 23 11

I'm open to discuss this: Do you consider this current behavior as "as designed", or are you open for "improvements"?

One way to enhance this would be to only use the low-res message when it's more than 1/2°C away from the last high-res message.

Cereal2nd commented 1 year ago

maybe its best to round this value always to the 1/2°C, this will give the most consistent data

Cereal2nd commented 1 year ago

or maybe ignore the temp status message if there is already data in the sensor so this would mean a small update to the update methode on a channel

niobos commented 1 year ago

maybe its best to round this value always to the 1/2°C, this will give the most consistent data

That would be consistent, but you'd loose the high precision output.

or maybe ignore the temp status message if there is already data in the sensor

This is kind of what I proposed: If the current data in memory is reasonable with the received low-res data, ignore the update; but if the low-res data indicates a (big enough) change in temperature, do update nevertheless.

Cereal2nd commented 1 year ago

Go for it, the second part. It's a relative simple fix

But please add a testcase for this, I want to go and try to add all new features in tests, this will give more confidence in the future. If we already do the new parts in testcase it will make the catch-up simpler.

Cereal2nd commented 1 year ago

@niobos are you already working on a fix for this? if not i'll start working on one

niobos commented 1 year ago

No, not started yet. I need to do some experimentation first, since it looks like the 1/2C message is not (always?) the rounded value of the 1/64C message.

Feel free to start already, I'll push commits as soon as I have something. That way you know when I start.

niobos commented 1 year ago

It looks like the temperature in the Temperature Sensor Status message is the truncated version of the temperature in the Temperature Sensor message. I.e. rounded down (at least for positive temperatures):

Screenshot 2022-10-30 at 14 11 21

However, I can't find an algorithm that does "the right thing" in all circumstances. I'll be thinking about this further...

niobos commented 1 year ago

To clarify some examples. The "Message temp" column indicates the value is the message; a 1-decimal-digit message indicates a Temperature Sensor Status message, a 4-decimal-digit message indicates a Temperature Status message. Bold values indicate unwanted behavior.

  1. State 1 is the current de-facto behavior: it keeps the value of the most recent message
  2. State 2 was the idea pitched above: Only update if the message is contradicting the stored value. This breaks when temperature is going down
Actual temp Message temp State 1 State 2
20.4375 20.0 20.0 20.0
20.4375 20.4375 20.4375 20.475
20.4375 20.0 20.0 20.475
20.4375 20.4375 20.4375 20.475
20.5000 20.5 20.5 20.5
20.5625 20.5625 20.5625 20.5625
20.6250 20.5 20.5 20.5625
20.5625 20.5625 20.5625 20.5625
20.5000 20.5 20.5 20.5625
20.5000 20.5000 20.5 20.5
20.4375 20.0 20.0 20.0
20.4375 20.4375 20.4375 20.4375

I would like to come up with an algorithm that works in the following scenarios:

  1. When only Temperature Sensor Status messages (1/2C) are received
  2. When only Temperature Status messages (1/64C) are received
  3. When a mixture of both are received, ideally getting the same performance as simply ignoring the 1/2C messages. (Simply ignoring would not work in scenario 1.)
Cereal2nd commented 1 year ago

the other solution is to always round the value received to 1/2C, then it will always be correct. But this will make the sensor less accurate, but i think its ok

ggaljoen commented 1 year ago

Maybe a late (stupid?) suggestion; to have the choice to use either one or the other? I prefer the more specified for more details.

Cereal2nd commented 1 year ago

Maybe a late (stupid?) suggestion; to have the choice to use either one or the other? I prefer the more specified for more details.

this would involve a lot of work, but its possible

1- implement the options flow in the integration 2- pass the options from the options flow towards velbusaio 3- if this param is set either round the 1/16C towards 1/2C or ignore the 1/2C messages

niobos commented 1 year ago

the other solution is to always round the value received to 1/2C, then it will always be correct. But this will make the sensor less accurate, but i think its ok

That would be correct, but unacceptable for my use-case.

I've implemented something I find acceptable in #52. Feedback welcome there. I'm closing this issue.