Hurricos / realtek-poe

7 stars 10 forks source link

realtek-poe: add ubus port power control #24

Closed bmork closed 2 years ago

bmork commented 2 years ago

Allow power to be dynamically disabled/enabled on individual ports, similar to the ancient lua implementation

Note that "port" parameter now takes the configured port name instead of the one-based index used by the lua code.

Signed-off-by: Bjørn Mork bjorn@mork.no

bmork commented 2 years ago

I've updated the patch with all the requested changes I know how to do.

I really don't know how/where to document the ubus interface. AFAIK, this is how all our ubus interfaces are documented:

root@gs1900-10hp-f:~# ubus -v list poe
'poe' @3ca0070f
        "info":{}
        "reload":{}
        "sendframe":{"frame":"String"}
        "manage":{"port":"String","enable":"Boolean"}

I arbitrarily renamed the call to "manage" in an attempt to describe the action, as requested. I believe "enable" would limit this interface unnecessarily in case we want to modify other per-port settings. Therefore "manage", which should uniquely define what this does along with the "enable" paramater. If there are use cases, then we can add more parameters to the same call.

bmork commented 2 years ago

OK, I ended up agreeing that there is little value in the distinction between "valid" and "enable". Removing it makes the code much more compact.

Made one additional unrequested change: Returning the result of poe_cmd_port_enable(), similar to what the "sendframe" method does. Not 100% sure about this since it means we return 0 instead of UBUS_STATUS_OK, but I consider consistent behaviour more important.

Hurricos commented 2 years ago

If @mrnuke says it's good, it must be good. I have left my GS1900-24HPv1 unplugged after constructing an external serial connector for it, but once it's plugged in again I will test, Tested-by and merge.

Hurricos commented 2 years ago

I was about to test this, then I found this bug, and then I had a family emergency and needed to rush home.

Anyways. I don't think at this point I need to hold this up. Merging.