DheerajKhajuria / pimatic-mysensors

mysensors
http://forum.mysensors.org/topic/797/pimatic-mysensors-controller-plugin
GNU General Public License v2.0
23 stars 24 forks source link

Bug in / Proposal for value handling of Multi-Sensor #39

Closed sebastianha closed 8 years ago

sebastianha commented 8 years ago

Currently the multi sensor only emits an event when the new value is different from the last one. See line 829 of MySensors.coffee

# If the received value is different then the current value, it should be emitted
@_setAttribute name, value

In my opinion this is not a good behavior, as duplicate values are therefore not stored in the DB. But this is important information that there has been a value. Consider a value which is the same for a long time. Then you don't know whether a sensor has not delivered the data or if the data is just the same (when looking at the DB only).

I propose to log every value which is received by the multi sensor. BTW: the other sensors do also log every value.

What do you think?

PascalLaurens commented 8 years ago

@sebastianha I agree that it would be better to store every received value. Are you going to make a pull request for that?

mwittig commented 8 years ago

I tend to disagree as this will create loads of redundant data. Instead, the sensor should have a failure state if it has not received data for a certain number of intervals. Well, may be it is the best to allow both, by making it an option. This way, the user can decide how to use it!

PascalLaurens commented 8 years ago

Yes, it's probably the best option to let the user decide. However, I hope it doesn't become to complicated for the user, with all the options.

Personally, I would adjust the Arduino sketch, so you don't send the enormous amount of redundant data in the first place. But I'm aware that not all users can or are willing to do that.

sebastianha commented 8 years ago

The thing is that the received data is not redundant!

At first it is the information that there has been data collected and second there is the timestamp information which is also important. Also the amount of the data isn't that large that it could not be stored easily.

The idea with the failure state would be an acceptable solution for some users but think about sensors which do not report regularly. For these sensors this is not a applicable.

Overall the default should be to store every value received and there could be an option to discard duplicate data with a "timeout" value after which a value is stored again even it is the same as the last.

For example when the timeout is 5 minutes: 10:00 | Temperature: 15C -> value is logged 10:01 | Temperature: 15C -> value is NOT logged 10:04 | Temperature: 15C -> value is NOT logged 10:05 | Temperature: 15C -> value is logged 10:07 | Temperature: 15C -> value is NOT logged ...

But these option should apply to all sensor types, not only to the multi sensor and should be optional with default to log everything.

This would be my solution to satisfy all needs.

mwittig commented 8 years ago

I am not sure why you want every value in the database.

So, I would really like to understand your application where you need every sample received from the sensor.

NB: I thin the timeout solution is difficult to implement as it can't be done as part of the plugin. A simple flag in the config whether I would like to record every sample or just state changes will do. This is very easy to implement and I am happy to draft an example for it. I can also do a PR but I have no mysensors setup at the moment to test with.

sebastianha commented 8 years ago

First of all I need always all data I can gather :)

In times of tera-byte sized sd-cards and quad-core embedded devices I don't see the point not so save any data which can be gathered.

Second: (as far as I remember when I first saw the problem) the graphs are not correctly rendered. When there is no data the graph will not insert a data point at the corresponding time. So if you have 10°, 10°, 10°, 10°, 20°C the graph will draw a straight line from the first 10° up to the 20° which is wrong! Correct would be a horizontal line over the first values and then from the last up to the 20°.

As I need only the option to keep every value I would be fine with your solution to make a flag which enables the discarding of duplicates but we should warn the user about the consequences for example the graph problem or other side effects I am not aware of.

sebastianha commented 8 years ago

Here is an example for the problem: screenshot113

mwittig commented 8 years ago

Thank you very much for your clarification. You are absolutely right for the common case! As the grapher of the pimatic-mobile-frontend (and must other tools) do not handle sparse (sensor) data this will lead to wrong results, but this also depends on time scale, obviously.

Depending on you are trying to achieve and the type of data you will need the full dataset rather than sparse data. Here are my use cases where sparse data makes sense;

I agree, triggering each value should be default. Adding an option to trigger changes only only adds one line of code and requires a change of one condition.

sebastianha commented 8 years ago

I agree on you with this.

So for a first implementation let's add an option which can enable the discarding of duplicate data and perhaps in future there could be a more sophisticated version which also takes into account the missing data without storing it explicitly.

As I did not spend much time with the code yet, could one of you implement the config option? That would be nice!

PascalLaurens commented 8 years ago

I agree on an option with a default of logging all incoming data too.

@mwittig Are you going to write the code? I've got an Arduino Nano with nfr24l01, so I can send fake sensor data, to test it.

sebastianha commented 8 years ago

Are you on Linux? Then you can emulate a serial input with the following trick:

~> socat -d -d pty,raw,echo=0 pty,raw,echo=0
2015/09/10 10:09:48 socat[19318] N PTY is /dev/pts/2
2015/09/10 10:09:48 socat[19318] N PTY is /dev/pts/4

Use /dev/pts/2 for pimatic config and

echo -e "NODEID;SENSORID;1;0;0;11.0\n" > /dev/pts/4 #temperature

to send data.

mwittig commented 8 years ago

@PascalLaurens, will do :)

PascalLaurens commented 8 years ago

@sebastianha Thank you for sharing that trick.

@mwittig Thank you :+1:

sebastianha commented 8 years ago

Why did you close this ticket? As far as I can see the suggested patch has not been written / applied yet.

sebastianha commented 8 years ago

Anything new here?