-
Notifications
You must be signed in to change notification settings - Fork 686
Add sensor component container #1369
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
base: master
Are you sure you want to change the base?
Conversation
|
/gcbrun |
|
No major YANG version changes in commit ec04f7f |
|
As more and more structures are proposed under the There are also |
| } | ||
|
|
||
| container state { | ||
| config false; |
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.
config false is inherited from parents. Is there ever intention for configurable thresholds (there are other variants of this in OC modeling as well in platform/system)
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 can imagine a scenario where one might want to configure these thresholds. Consider if the management system wanted to tune when an alarm might fire due to the sensor being out of range. I don't have this usage in mind. The use case I'm modeling for is to have these thresholds be vendor supplied, so config false seems appropriate. Thoughts?
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.
the state container should be config false always. But don't label the parent container config false. Allow for a config container to be added later which would allow user configurable thresholds.
Today all the operational use cases I am aware of do not require a user override of factory defined thresholds for hardware components.
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.
Removing this
list threshold {
key "severity";
config false; <<<<<<<<
... requires adding the key into a config container, just FYI, thanks.
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.
Ignore that comment... transceiver provides an example where this isn't true.. sorting out this error:
The list's keys must have the same
configvalue as the list itself.(invalid-config)
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.
Maybe I'm not understanding the comment. The thresholds container in the transceiver container also has config false:
| config false; |
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.
The latest change applies a when on sensor/state at L177.
| augment "/oc-platform:components/oc-platform:component/" + | ||
| "oc-platform:sensor" { | ||
| description | ||
| "Adding fan data to component model"; |
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.
Miscopy from another model?
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 can swear I addressed this at some point... losing my mind. Good catch, thanks!
| augment "/oc-platform:components/oc-platform:component/" + | ||
| "oc-platform:sensor/oc-platform:state" { | ||
| description | ||
| "Adding fan state data to component model"; |
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.
Miscopy?
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.
Good catch, thanks.
Great comment. The intention is for this container to only be used when type=SENSOR. From a quick search, looks like this can be accomplished with a |
Similar example here: https://github.com/openconfig/public/blob/master/release/models/platform/openconfig-platform.yang#L1350-L1355 |
|
/gcbrun |
In order to access
The following snippet... ... resulted in this warning: The relocation worked... so I'm going with it. Updating the description to capture this change. |
I think your prior anchor point was the more correct location as this follows the same pattern for other "type" hierarchies... just that many of these containers today have no schema rules of when they should or should not exist (left up to interpretation) Your compiler issue stems from interleaving a config=true container to attempt to depend on config=false. One option is leave as-is w/o |
Update release/models/platform/openconfig-platform-sensor.yang Co-authored-by: Ebben Aries <[email protected]> Update release/models/platform/openconfig-platform-sensor.yang Co-authored-by: Ebben Aries <[email protected]> Add when predicate to sensor, address comments remove whitespace
I've reverted to the original anchor point (component/sensor), and moved the |
|
This model could be very useful for power supplies that have multiple inputs and/or multiple outputs -- we could represent the separate readings as distinct sensor subcomponents instead of complicating the power-supply model. In that kind of scenario, it would be helpful to represent the property of the sensor's "directionality". What do you think about adding a path that captures that -- for example, allowing us to distinguish between input current and output current? |
| description | ||
| "Operational state data for sensor components"; | ||
|
|
||
| leaf instant-precision2 { |
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.
What's the rationale for including the precision in the name of the leaf? Is the idea that a different name would be used if a different precision were needed? If so, wouldn't that be contrary to the aim of a generic sensor model?
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.
Exactly. I wanted to accommodate different precision requirements should they arise in the future. From a survey of existing sensor values, "2 decimal places" covers the current use cases. Should another use case come by requiring another level of precision, I wanted to avoid the scenario where "instant" carried some implicit precision and exists among "instant-precisionX".
If other precisions are required, the expectation should be that only a single "instant" should be populated.
|
/gcbrun |
|
The component type SENSOR already exists. So prior to this PR, implementations would have been expected to report, for example, temperature in the path /components/component/state/temperature/instant if they were reporting it against a SENSOR component. Is it being proposed to report something like temperature inside the /components/component/sensor container now? |
Change Scope
sensorcontainer to/components/component[name=]/state. This container defines an instantaneous value along with operating thresholds.It's worth highlighting, the units of the sensor's instantaneous value are defined by a parallel leaf, allowing this generic sensor component to represent multiple sensor types (current, voltage, ect).
Platform Implementations