Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
zio: initial channel definitions #14245
zio: initial channel definitions #14245
Changes from 1 commit
6239b74
aadfa33
926dff9
1ee840d
d57cd9b
ab15598
c2ad7ba
4fe8819
1c15e31
bb61ff8
32f7fa8
303384c
696d9d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any reason to have 3 types of temperature?
Aren't DIE and AMBIENT enough?
I'm not against it. Just want to understand the reason.
Maybe we could have only two types: AMBIENT and GENERAL. The DIE case would fall into the GENERAL case. Or maybe we can keep it this way.
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.
Many sensors out there include temperature measurement, but it's often only of the die so of less use to the outside world, and I guess I just wanted to clearly distinguish die temp (which could me many degrees different than ambient temp or the same measurement on another IC), from ambient which is device nuetral and closer to what people want, without ignore the need to sometimes track die temp for either generating appropriate offets for the sensor data, or for safety reasons.
The third (first?) generic temp entry --
DEV_CHAN_TEMP
-- is basically for anything that isn't the two most common cases of DIE or AMBIENT, and you could then define exactly what that is via attributes (water temperature, heating-element temperature, whatever).I'm open to change this, just trying to explain the reasoning in my own head!
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.
Yes, I agree.
Actually if you see the history of commits of the current sensor.h you'll see that I'm the one that separated the two temperature types. So, yes, I agree with this part.
I was in fact more questioning about this third type.
We can keep it, I'm not against it. Maybe it is more clear like that, but we cannot have as many
temperatures definition as are out there. So, I was thinking to have only two types: AMBIENT and TEMP which means other non ambient. But, again, maybe this way it is clearer.
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 that the number of axis should be specified in a different parameter.
The channel type should be always ACCEL (or GYRO or MAGN).
So, no need for a different type. Does it make sense to you?
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.
It does and I'm not in love with the concept of vector, my concern is that with sensor fusion you often want to be sure that the three values come from the same sample or conversion process, and if you have three different channels for X+Y+Z, you need to add a means to associate those values with each other, which becomes more code to validate.
The need to group multiple channels will come up elsewhere, of course, such as in light sensors with multiple photo diodes, each with their own channel, though that takes the approach of having a single channel per photodiode and the attributes fill the details in.
How would you see the problem of needing to keep X/Y/Z related to each other so that you don't get X+Y from one conversion, but Z from the next?
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 need to see more carefully the example of Google CHRE, where they define the sensor type as ACCEL or GYRO or MAGN only, but also define in the sensor property the number of axis supported by the sensor (One of SINGLE, TRIPLE, EMBEDDED, with latter one used for temp/baro/humidity/stepc and so on).
Then a sensor when generates samples enqueue above them as events, and a general task would consume them. This is different from how zephyr works now where samples are pulled from above and not pushed from below as events.
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.
If you have some suggestions here, it will never be easier than now to make the right decisions and tradeoffs!
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.
Yes, sure.
I'll try to get back to this asap.
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.
Here I would keep hPa as unit (which equals mbar btw), just to be consistent with the usual unit.
Would DEV_CHANNEL_PRESS_AMBIENT be a better name?
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.
Ambient might actually be a clearer name, yes.
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 would prefer
DEV_CHANNEL_PRESS_ABSOLUTE
andDEV_CHANNEL_PRESS_RELATIVE
, and perhapsDEV_CHANNEL_PRESS_GAUGE
.Most pressure sensors, e.g. BME280 or MS5637, produce absolute atmospheric pressure, which would range from about 1068 hPa down to 314 hPa. Others like SDP810 produce relative pressure in a differential application, such as output and return in an HVAC system, which is often on the order of 10-25 Pa (with precision down to cPa) and can be negative.
Gauge pressure would be like a tire pressure sensor: relative to absolute atmospheric pressure.
Terms like "barometric" or "atmospheric" may be interpreted as referring to absolute pressure adjusted for altitude, like you get in weather reports. This can be very different from absolute pressure. Barometric pressure could be a derived measurement, but having a sensor that measures it directly seems pretty unlikely (maybe one built into a GPS). Avoiding those terms might reduce confusion.
As for units: I'd really prefer SI throughout, so Pa. Let the consumer deal with scale conversions.