Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dajtxx
Copy link

@dajtxx dajtxx commented Dec 9, 2022

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
Copy link

CLAassistant commented Dec 9, 2022

CLA assistant check
All committers have signed the CLA.

@pablojimpas
Copy link
Contributor

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
Copy link
Author

dajtxx commented Dec 22, 2022

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.

lib/payload.json Outdated
Comment on lines 130 to 142
"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": {
Copy link
Member

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?

Copy link
Author

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 Show resolved Hide resolved
lib/payload.json Outdated
Comment on lines 51 to 56
"ec": {
"type": "number",
"description": "Electrical conductivity (dS/m)",
"minimum": 0,
"maximum": 621
},
Copy link
Member

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

@pablojimpas
Copy link
Contributor

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
Copy link
Member

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

@dajtxx
Copy link
Author

dajtxx commented Jun 12, 2023

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).

Bring DPI fork up to date with TTN
@pablojimpas
Copy link
Contributor

pablojimpas commented Jun 13, 2023

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:

  • wind.speed: As it was before, used for the current speed.
  • wind.averageSpeed: Used for the continuous average during the entire uplink interval.
  • wind.gustSpeed: I prefer this over just wind.gust, is more descriptive and makes more sense if maybe in the future more related fields are introduced, like wind.gustDuration for example.

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
Copy link
Author

dajtxx commented Jun 13, 2023

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.

Copy link
Member

@johanstokking johanstokking left a 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)",
Copy link
Member

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

Suggested change
"description": "Salt (mg) per litre (ppm)",
"description": "Salt (mg) per liter (ppm)",

Copy link
Contributor

@pablojimpas pablojimpas left a 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)",
Copy link
Contributor

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.

Suggested change
"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)",
Copy link
Contributor

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:

Suggested change
"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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants