dbus-sensors:hwmontemp: additional attribute proposal

[Jason Ling]

>
>  don't like the merging of base lists with extended lists, as it adds
> a dependency between how we represent that, and implies that we have a
> published plugin interface, which we definitely don't, nor do we want
> to maintain it at the entity manager level.  It also means that
> upstream never tests the "extended" list, which means it's a lot more
> likely to break.
>
My stance on devices added to the extended list is the same as devices
added by downstream patches. Upstream maintainers aren't responsible for
testing those, if you're patching in devices then you take the
responsibility of testing those.
My objective is to make it so devices that we don't want to upstream yet
can be maintained more easily.
First idea was a json file that extends types/lists but there are concerns
with runtime parsing for devices for the purposes of exposing telemetry
(I'd think runtime parsing of things like PID configurations would be more
worrisome).
So second idea was just to move data structures around in
dbus-sensors/PSUSensor to make upstream changes less likely to result in
merge conflicts for those who are maintaining downstream patches.
Furthermore just split the data structure up into a portion that is
upstream and downstream.
Yes, it's definitely making it friendlier for those who want to maintain
downstream patches to extend devices. I don't see how this increases
maintenance or testing burden for the maintainers though.
If however , the intent is to explicitly send the message
"You shouldn't try to use this service for any devices that do not have
explicit upstream support. Any patches that make it easier to do so will be
rejected."
then I agree with your earlier suggestion that maybe the best approach is
to create another service for those devices.

I originally wrote a big long idea about how to make the above work,
> but got to the end, and realized that I'm still trying to understand
> what we're trying to avoid here with the downstream/upstream lists
> thing.  It's easy enough to patch the existing list to add your new
> custom types, then when it comes time to merge because the
> project/component is public, the patch is ready and good to upstream.
> What are we buying by moving that info to a non-patch format?

You get the benefit of separating devices into two classes..
(1) types that are upstream , have been tested by someone else and they are
ready to go without additional work.
(2) types that are not upstream, because the devices are not public yet or
may never be public and need to be kept downstream for a long period of
time (or forever).

> I think
> moving it to a file means it's a lot less likely to be upstreamed, as
> it requires the next person to understand it to use it, and modify the
> patch rather than simply cleaning up the commit message, testing it,
> and firing it at gerrit.

Yes, the file would be for those things that are never meant to be
upstreamed or won't be upstreamed for a long while.

> Having done this pattern many times
> (including with that list specifically) I think the only thing we
> could improve would be to move that list to its own file (but still
> C++ code), so it gets fewer merge conflicts, and you can completely
> replace it if you like, but even that doesn't solve the problem if
> code is never upstreamed.

Yup, the problem here are the following
"I have patches I keep downstream and they keep getting broken whenever the
types/device list gets updated. I wish these data structures were in a file
that doesn't get patched often"
and
"I have patches to add devices to the type/list data structure and I can't
upstream them for a long time (or ever). I don't want to waste time
constantly fixing my broken patches everytime someone adds a new public
supported type."
Both approaches (parsing json and extending the list runtime and separating
the data structs into a separate file + returning the union of base +
extended) accomplish the same thing. One requires a recipe to copy a
ExtendPSUSensorConfig.json in a recipe somewhere to usr/share/PSUSensors
(or something) and the other is just a patch that gets applied. Talking it
through, I now realize that the slight code refactoring approach is a lot
less work and a lot more simple..and something that I'd actually have time
to contribute.


Have you filed a bug, or asked on the mailing list before now?  This
> is the kind of feedback the authors of that sensor need (Ideally
> before you move over to another subsystem like hwmontemp).

I never really considered hwmontemp a different sub-system since they live
in the same repo and seemed to be more specific towards monitoring
temperature telemetry.
As far as bugs go, I CC'ed Alex Qiu who has more experience with the
performance of PSUSensors. I haven't personally ran into this problem; just
know about it from talk amongst the larger team.

> If I
> didn't see your message/bug and you did post it, I apologize, I'm not
> trying to call you out.
> If you have specifics, like the value of N, and the details around
> what chips you're interacting with and whatever debugging you've done,
> it would be helpful to put that in a bug for triage.
>
Alex, maybe you can add some color here?

> Keep in mind, PSUSensor by default has a 1 second scan rate.
>
> https://github.com/openbmc/dbus-sensors/blob/41061e2c3198c0f597d4f6bb702b690a273ab45d/include/PSUSensor.hpp#L38
> If it's not obeying that, clearly there's a bug to fix somewhere.
> On some platforms, I have seen very high rate polling of power values
> on the PSU I2c bus by other devices, and that tends to hold up
> transactions for other components.  If that bus is misbehaving or
> overloaded on your platform, it might have triggered a weird condition
> within the PSU sensor (like the scans taking longer than the scan
> rate).
> If you have any more details here, it's quite possible someone will
> have an idea where to look, or know exactly where the problem is.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200807/7a2d2458/attachment.htm>