TheThingsNetwork / lorawan-devices

Device Repository for LoRaWAN devices
Apache License 2.0
192 stars 370 forks source link

Split wind speed into avg & gust, added water measurements. #568

Open dajtxx opened 1 year ago

dajtxx commented 1 year ago

Summary

The AWS we use reports both the average wind speed over the measurement interval, plus the maximum speed. I believe this is reasonably common and the average wind speed takes the place of the current single wind measurement.

We have many water sensors, both in freshwater tanks and troughs and in an estuary so I added measurements for those.

The electrical conductivity type of measurement is reusable now.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

pablojimpas commented 1 year ago

I don't know how I feel about the gust and avg speed, what if the device only reports a single value with the current measurement? Will it map to gust speed? We have already discussed somewhere in the normalization issue the need for grouping and statistical values to save bandwidth, but I don't recall what was decided @johanstokking

The electrical conductivity type of measurement is reusable now.

I initially put the electrical conductivity definition specific to the soil object to give a more meaningful value with the correct units for that domain. I don't know if dS/m is fine for your domain (water measurements) but we discussed earlier that it makes sense abstracting definitions to be reusable when they are in SI units and can be applied to a wide range of domains, otherwise we could use specific definitions. If these units make sense for water measurements, I'm fine with this change for now, at least until we have to add more use cases and the reusable definition becomes less useful.

dajtxx commented 1 year ago

I don't know how I feel about the gust and avg speed, what if the device only reports a single value with the current measurement? Will it map to gust speed?

I think it would map to average speed. I don't know how ultrasonic wind sensors work but I think the more usual pulse counter would be very unlikely to count two pulses and call it good.

Plus, unless I have it wrong again with JSON schema, isn't there still a key to tell you what the value is - "avgSpeed" vs "gustSpeed"?

I don't know if dS/m is fine for your domain

As far as I can see the sensor is using EC to measure salinity, and reporting a number for the EC measurement in the SDI-12 values, but I do not know what unit it is because I cannot find a datasheet for the C4E. If I knew the unit being reported it would just be a matter of multiplying or dividing to get to whatever the SI unit for EC is.

So whatever the SI unit for EC is, we should use it and I can make our decoders convert to it.

pablojimpas commented 1 year ago

I think it would map to average speed. I don't know how ultrasonic wind sensors work but I think the more usual pulse counter would be very unlikely to count two pulses and call it good.

Plus, unless I have it wrong again with JSON schema, isn't there still a key to tell you what the value is - "avgSpeed" vs "gustSpeed"?

I've contacted with Seeed to ask how wind speed is implemented in S2120, and they told me that it's averaged over a 12-second period. I think that naming that reading “average” in the normalized payload can be misleading, since one may infer that it's the average during the uplink interval period (typically much higher than 12 seconds).

So, I think that in the spirit of backwards compatibility we can just leave the name as it is wind.speed. Maybe clarify in the description that it is a value at a given time, but that it may be averaged over a small period of seconds to get a more precise reading.

The gust speed is a nice addition anyway. Furthermore, one may argue that some devices aren't sleeping in between uplinks and that they report the average during the complete uplink interval, in that case, we can add the new field for averageSpeed with this clarification in the description.

johanstokking commented 1 year ago

THanks @pablojimpas. @dajtxx can you pick this up as part of this PR?

dajtxx commented 1 year ago

The gust speed is a nice addition anyway. Furthermore, one may argue that some devices aren't sleeping in between uplinks and that they report the average during the complete uplink interval, in that case, we can add the new field for averageSpeed with this clarification in the description.

The Atmos41 AWS we use is continuously powered and samples the wind speed every 10 seconds and calculates the average of these readings when you read from it, then resets the averages and starts again. So it is exactly as you describe above.

I had speed, avgSpeed, and gustSpeed above and it didn't seem suitable at the time. Has the conversation brought that idea back into consideration? Or would you prefer speed and gustSpeed?

I'm happy with wind.speed and wind.gustSpeed (or wind.gust).

pablojimpas commented 1 year ago

The Atmos41 AWS we use is continuously powered and samples the wind speed every 10 seconds and calculates the average of these readings when you read from it, then resets the averages and starts again. So it is exactly as you describe above.

In this case, it makes sense to have an averageSpeed field, yes. But it should be indicated somewhere in the description of the field or in the documentation of the normalized payload format that this field is for measurements that represent the average during the whole uplink interval period. If the device calculates the average during a small period, like the 12 seconds of the SenseCAP S2120, then this averageSpeed field must not be used, that will be just a regular measurement of the current speed.

I will suggest the following naming:

However, adding this kind of average field creates a precedent, and it can be replicated in every field. Perhaps we have to rethink the schema to introduce a grouping/stats system; otherwise, I foresee a future in which we pollute the schema with quantityX, averageQuantityX, maxQuantityX, minQuantityX, stdDevQuantityX… for every single value. Once you start grouping, time becomes even more relevant, knowing the uplink arrival time might not be enough because if you want the stats to be useful you must know the start and end timestamps of the “time bucket”. It is not safe to always assume that start time = uplink arrival time - uplink interval time, since packets may be lost or the device might follow different cycles for reading from sensors and for sending uplinks.

dajtxx commented 1 year ago

However, adding this kind of average field creates a precedent, and it can be replicated in every field.

I was going to write something similar, having let the idea sit overnight.

I think averageSpeed can be dropped. Use speed for the average speed reading from the ATM41 and still add 'gustSpeed' for that actually new and different measurement.

I'll make that change.