-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split wind speed into avg & gust, added water measurements. #568
base: master
Are you sure you want to change the base?
Conversation
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
I initially put the electrical conductivity definition specific to the |
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"?
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. |
lib/payload.json
Outdated
"speed": { | ||
"description": "Wind speed (m/s)", | ||
"avgSpeed": { | ||
"description": "Average wind speed (m/s)", | ||
"$ref": "#/definitions/speed" | ||
}, | ||
"gustSpeed": { | ||
"description": "Gust wind speed (m/s)", | ||
"$ref": "#/definitions/speed" | ||
}, | ||
"direction": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change. I think it is indeed useful domain knowledge to differentiate between average and gust wind speeds.
Can you turn it into averageSpeed
though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is done.
lib/payload.json
Outdated
"ec": { | ||
"type": "number", | ||
"description": "Electrical conductivity (dS/m)", | ||
"minimum": 0, | ||
"maximum": 621 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now unused in water
so it can be reverted
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 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 |
THanks @pablojimpas. @dajtxx can you pick this up as part of this PR? |
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). |
Bring DPI fork up to date with TTN
In this case, it makes sense to have an 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 |
I was going to write something similar, having let the idea sit overnight. I think I'll make that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, thanks. @pablojimpas let me know what you think.
"$ref": "#/definitions/pH" | ||
}, | ||
"salinity": { | ||
"description": "Salt (mg) per litre (ppm)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're following en-US
"description": "Salt (mg) per litre (ppm)", | |
"description": "Salt (mg) per liter (ppm)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @johanstokking, apart from some minor wording. We can add this for now and design a grouping and statistics system later.
@@ -128,7 +128,11 @@ | |||
"type": "object", | |||
"properties": { | |||
"speed": { | |||
"description": "Wind speed (m/s)", | |||
"description": "Instantaneous or average wind speed (m/s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid documenting/promoting the use of average here until we figure out a way to introduce grouping and stats.
"description": "Instantaneous or average wind speed (m/s)", | |
"description": "Wind speed (m/s)", |
"$ref": "#/definitions/speed" | ||
}, | ||
"gustSpeed": { | ||
"description": "Maximum wind speed during the measurement interval (m/s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not making assumptions on how the gust speed is calculated:
"description": "Maximum wind speed during the measurement interval (m/s)", | |
"description": "Gust wind speed (m/s)", |
Bringing DPI fork up-to-date with source repo
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.