<div dir="ltr"><div>Hi Ed and James,</div><div><br></div><div>Is it acceptable for a device to be listed in both HwmonTempSensor and PSUSensor?</div><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">- Alex Qiu</div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Aug 7, 2020 at 2:53 PM Ed Tanous <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Aug 7, 2020 at 2:05 PM Jason Ling <<a href="mailto:jasonling@google.com" target="_blank">jasonling@google.com</a>> wrote:<br>
>><br>
>>  don't like the merging of base lists with extended lists, as it adds<br>
>> a dependency between how we represent that, and implies that we have a<br>
>> published plugin interface, which we definitely don't, nor do we want<br>
>> to maintain it at the entity manager level.  It also means that<br>
>> upstream never tests the "extended" list, which means it's a lot more<br>
>> likely to break.<br>
><br>
> 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.<br>
<br>
Sure, but upstream does bear the responsibility for testing that this<br>
"extended types" system works?<br>
<br>
> My objective is to make it so devices that we don't want to upstream yet can be maintained more easily.<br>
<br>
Ever?  To be frank for a moment, you're going to have a hard time<br>
pushing that one forward in an open source project.  If there are a<br>
lot of users that are planning on maintaining private forks forever,<br>
maybe there's a case for this, but I think most uses of private forks<br>
tends to be temporary, even if it's longer term, and the long term<br>
intention is to upstream.<br>
<br>
> 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).<br>
> 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.<br>
> 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.<br>
<br>
It increases burden because any time the maintainer wants to change<br>
the name of that file, change the name of the structure in that file,<br>
add a field, rename fields, or change compiler flags, that file will<br>
break in your downstream, and the maintainers will have no idea.  It<br>
definitely increases the maintenance burden, and all of the things<br>
I've mentioned and more have happened over the short life of entity<br>
manager.  I'm sure they will continue to happen as they evolve (or<br>
until EM is replaced by something better).<br>
<br>
> If however , the intent is to explicitly send the message<br>
> "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."<br>
> then I agree with your earlier suggestion that maybe the best approach is to create another service for those devices.<br>
<br>
I'm not the maintainer of this project anymore, so my opinion is just<br>
that, and I have no ability to reject patches :)  I would rephrase my<br>
position as: Modifying the source code directly is not an adequate<br>
long term API for making permanent, never to be upstreamed changes.<br>
The closest guarantee to that kind of API that is dbus, the second<br>
closest is an Entity manager config, each of which have their own pros<br>
and cons.  If we as a project can do anything to make the transition<br>
between downstream code to upstream code easier, like trying to make<br>
merge conflicts less likely on commonly modified files, without the<br>
expense of maintainability or complexity, I'm absolutely for it, but<br>
creating explicit data structures and hard guarantees about downstream<br>
code on a binary boundary needs a much larger discussion, and speaks<br>
to some of the project's main principles about the "Open" part of<br>
OpenBMC.<br>
<br>
><br>
>> I originally wrote a big long idea about how to make the above work,<br>
>> but got to the end, and realized that I'm still trying to understand<br>
>> what we're trying to avoid here with the downstream/upstream lists<br>
>> thing.  It's easy enough to patch the existing list to add your new<br>
>> custom types, then when it comes time to merge because the<br>
>> project/component is public, the patch is ready and good to upstream.<br>
>> What are we buying by moving that info to a non-patch format?<br>
><br>
> You get the benefit of separating devices into two classes..<br>
> (1) types that are upstream , have been tested by someone else and they are ready to go without additional work.<br>
> (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).<br>
<br>
Don't you already have that distinction today?  Devices that are in<br>
your downstream patch fall into category 2, devices that are on<br>
openbmc master fall into category 1.  Maybe I'm missing something?<br>
<br>
>><br>
>> I think<br>
>> moving it to a file means it's a lot less likely to be upstreamed, as<br>
>> it requires the next person to understand it to use it, and modify the<br>
>> patch rather than simply cleaning up the commit message, testing it,<br>
>> and firing it at gerrit.<br>
><br>
> Yes, the file would be for those things that are never meant to be upstreamed or won't be upstreamed for a long while.<br>
<br>
See above.<br>
<br>
>><br>
>> Having done this pattern many times<br>
>> (including with that list specifically) I think the only thing we<br>
>> could improve would be to move that list to its own file (but still<br>
>> C++ code), so it gets fewer merge conflicts, and you can completely<br>
>> replace it if you like, but even that doesn't solve the problem if<br>
>> code is never upstreamed.<br>
><br>
> Yup, the problem here are the following<br>
> "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"<br>
<br>
Moving that structure to its own file sounds totally reasonable,<br>
although (if I were the maintainer) it would have no guarantees<br>
granted.  Said file may change name, structure, naming, or compiler<br>
magic that will cause downstream to break.  Funnily enough, in the act<br>
of implementing that, you will ironically break a lot of peoples<br>
downstreams, and I'm personally ok with that.  You (and your team)<br>
needs to be ok when people break your downstream for similar reasons.<br>
<br>
On a personal note, if you haven't already, I highly recommend<br>
spending some learning time on getting a good setup and mental model<br>
for merging conflicting patches.  As a useful skill, it comes up<br>
ridiculously often in software, especially if you're a maintainer.  If<br>
you have the ability to resolve conflicts quickly and correctly it<br>
puts you at a significant time advantage to your peers that don't.<br>
<br>
> and<br>
> "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."<br>
> 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.<br>
<br>
EXCELLENT!  Add me to the patch, and I'd be happy to review it for you<br>
(With that said, James is pretty fast and sometimes beats me to the<br>
punch).<br>
<br>
><br>
><br>
>> Have you filed a bug, or asked on the mailing list before now?  This<br>
>> is the kind of feedback the authors of that sensor need (Ideally<br>
>> before you move over to another subsystem like hwmontemp).<br>
><br>
> 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.<br>
<br>
Ignore the above comment about subsystem.  I thought you were talking<br>
about phosphor-hwmon.  My bad.<br>
<br>
> 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.<br>
<br>
Excellent.  Looking forward to details.<br>
<br>
>><br>
>> If I<br>
>> didn't see your message/bug and you did post it, I apologize, I'm not<br>
>> trying to call you out.<br>
>> If you have specifics, like the value of N, and the details around<br>
>> what chips you're interacting with and whatever debugging you've done,<br>
>> it would be helpful to put that in a bug for triage.<br>
><br>
> Alex, maybe you can add some color here?<br>
>><br>
>> Keep in mind, PSUSensor by default has a 1 second scan rate.<br>
>> <a href="https://github.com/openbmc/dbus-sensors/blob/41061e2c3198c0f597d4f6bb702b690a273ab45d/include/PSUSensor.hpp#L38" rel="noreferrer" target="_blank">https://github.com/openbmc/dbus-sensors/blob/41061e2c3198c0f597d4f6bb702b690a273ab45d/include/PSUSensor.hpp#L38</a><br>
>> If it's not obeying that, clearly there's a bug to fix somewhere.<br>
>> On some platforms, I have seen very high rate polling of power values<br>
>> on the PSU I2c bus by other devices, and that tends to hold up<br>
>> transactions for other components.  If that bus is misbehaving or<br>
>> overloaded on your platform, it might have triggered a weird condition<br>
>> within the PSU sensor (like the scans taking longer than the scan<br>
>> rate).<br>
>> If you have any more details here, it's quite possible someone will<br>
>> have an idea where to look, or know exactly where the problem is.<br>
><br>
><br>
</blockquote></div>