dbus-sensors:hwmontemp: additional attribute proposal

[Ed Tanous]

On Fri, Aug 7, 2020 at 11:05 AM Jason Ling <jasonling at google.com> wrote:
>
>
>> Yeah that's what I meant, a generic PSUSensor Type with a field called
>> 'Export' or something that EM can use to get the Export type.
>
> Sure, I think that handles the EM side of things.
>
>> I'm conflicted on that.  Part of the reason that list is there, and
>> not in the config files directly, is to convey that those are the
>> types that have been tested to work correctly, and the types that the
>> config files "promise" to work sanely.  The other thing is, if you've
>> done the testing, adding to that list is (should be) relatively
>> trivial and straightforward.  Opening up that list to runtime parsing
>> opens a lot of security questions I'm not prepared to answer.
>
> Sure, let's scrap runtime parsing and go for something far simpler.

Unless you have a way to handle the cases where things need to be
configurable at runtime (like adding a new entity config or fan
control control without a recompile) I don't think we can "scrap"
runtime parsing entirely, but if you have a plan for dealing with the
aforementioned, I'd love to hear ideas!

> (1) have a base type, devices list that represents the approved device list.
> (2) have an empty extended type, device list that represents user specified extensions.
> (3) factor these out into separate files and provide a method that returns the union of the base and extended types/devices.
>
> now we have a base type/device list that contains supported guaranteed devices and another extended type/device list that is easier to maintain patches for.
> I think that's a rather small change and accomplishes what I'd like while preserving the intent of keeping a list of supported types/devices.
> What do you think?


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

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?  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.  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.  If you want to code that up, I'd support it
(and I'd guess James would too).  Keep in mind, all your downstream
patches against that list will break when you do that, causing the
world irony meter to spike :)   With that said, i still think it's
worth it.

Disclaimer time: The way the project recommends to avoid this problem
is to upstream your code.  Anything else is a half measure, and we can
make no guarantees about your downstream fork.