Change PoE type setting and autogenerate new configuration files

[svanheule]

In order to make the poe config file a bit more generic, which may require multiple types of PoE output types, this PR add two patches that first add support for a new 'poe_type' setting (deprecating 'poe_plus'), followed by a migration script.

Secondly, a script is provided to autogenerate configuration files based on board.json. This replaces the poe config file included in the realtek-poe package.

[svanheule]

A potential future change would be to also generate poe_type from board.json, but this requires more changes to 02_network in the OpenWrt repository. This should already be a good start to generate more sane configuration files.

[Hurricos]

I'm running out of time tonight, but I like the idea. I have a ZyXEL GS1900-24HPv1 or 2 to test with - I've executed the build check from which we should get a realtek .ipk artifact I can install-test.

I could actually test it remotely, but the switch is reachable over a wireless bridge powered by the switch ;^)

... @svanheule, would you expect the resulting .ipk to "just work" if I opkg install $ipk; reboot;? Worst-case scenario, it'll take me an extra 30 minutes to recover the connection if not.

[Hurricos]

... I'm retesting that build in an realtek-rtl838x-openwrt-23.05 SDK container ...

[mrnuke]

I think the contract of realtek-poe is to take a config file and control the hardware. We speak UCI config on one end, and the PoE protocol on the other.

I don't think it should be the job of realtek-poe to generate the config files. The question then arises: If we speak UCI. and JSON, why not take the JSON config in the first place?

[Hurricos]

I think the contract of realtek-poe is to take a config file and control the hardware. We speak UCI config on one end, and the PoE protocol on the other.

I think we follow the philosophy of "describe hardware, then parse description" wherever possible in other places in OpenWrt. This PR feels like a response to getting support-requests from folks who don't understand why their configuration isn't working out-of-the-box -- but ease-of-use is good for experienced users too.

I don't think it should be the job of realtek-poe to generate the config files. The question then arises: If we speak UCI. and JSON, why not take the JSON config in the first place?

Is that to ask, "why not read board.json directly"? More about standards-keeping IMO -- the model is, generate defaults from board.json, and from those generate conf files if none exist.

I agree that uci is very clumsy, but this is OpenWrt we're talking about.

[Hurricos]

Few errors @svanheule - rather than reviewing the changes directly, here's a patch that gets it to compile.

diff --git a/src/main.c b/src/main.c
index 20ed6dc..9994d2c 100644
--- a/src/main.c
+++ b/src/main.c
@@ -24,12 +24,12 @@
 typedef int (*poe_reply_handler)(unsigned char *reply);

 #define POE_TYPE_802_3AF   "ieee8023af"
-#define POE_TYPE 802_3AT   "ieee8023at"
+#define POE_TYPE_802_3AT   "ieee8023at"

 enum power_up_mode_t {
    POWER_UP_MODE_802_3AF = 0,
    POWER_UP_MODE_802_3AT = 3,
-}
+};

 /* Careful with this; Only works for set_detection/disconnect_type commands. */
 #define PORT_ID_ALL    0x7f
@@ -106,7 +106,7 @@ static void write16_be(uint8_t *raw, uint16_t value)

 static void load_port_config(struct uci_context *uci, struct uci_section *s)
 {
-   const char * name, *id_str, *enable, *priority, *poe_plus;
+   const char * name, *id_str, *enable, *priority, *poe_plus, *poe_type;
    unsigned long id;

    id_str = uci_lookup_option_string(uci, s, "id");

[svanheule]

I have to admit I didn't actually test the daemon changes. The scripts should be OK. I had these mostly-finished patches laying around for a few days now, so I mostly wanted to get them 'out there', if only for some discussion.

A good out-of-the-box experience I mainly what I'm aiming for with these patches. After upgrading to 23.05, I noticed my poe config file had disappeared after installing realtek-poe. It seems like @flygarn12 experienced something similar when testing openwrt/openwrt#14100. Even for experienced users, who know what they're doing, this type of issues is just a waste of time.

I think the main reason for not just using board.json, is that config/poe allows for more freedom in how to set things up. A board description doesn't tell you which ports should have priority when hitting the limit of the power budget. That's only something a config file can tell the daemon.

[svanheule]

@Hurricos I've applied your patch, so at least this should build now. I can test a generated .ipk as well, if you want.

[Hurricos]

@Hurricos my argument is that realtek-poe should not be the part responsible for converting JSON to UCI config. I think that's beyond the scope of realtek-poe.

Our mission is to turn on power to the ports given the user's wishes. as expressed by an UCI config. In my opinion, how that config gets there should not be of concern to the realtek-poe package.

Thanks; at least I know now I understood your position.

My position is that hardware-intertwined packages should generally be functional out-of-the-box, packaged with (or generating) a configuration that users can later modify -- think e.g. wpad. Our etc/config/poe is not functional out-of-the-box in most cases, which makes it harder for users to execute their wishes through changes to the configuration (they have no reasonable baseline!).

Hardware-intertwined userspace packages are rare, so there are few comparisons for other packages that do this.

There are other ways to achieve this -- the defaults shell-case-statements used by many packages (uboot-envtools, e.g.) is the most obvious one -- but I definitely don't want to make that the source of truth when board.json already expresses it.

... I say that, but to be clear @svanheule, how does board.json know the PoE power limit? Don't tell me that's all declared in a defaults script?

[mrnuke]

To the extent we want this functional out of the box, I agree. I think there are other ways to do it. In absence of a config file, can we turn on all the ports we detect? This way, you eliminate any dependency on a config.

The information in the JSON is derived from /etc/board.d/02_network. That's in OpenWRT proper. That's another reason I think the config should be given by OpenWRT proper.

This JSON config is part of board_detect and config_generate scripts. What happens if you delete board.json, then install realtek-poe? There are a lot of nuances of how OpenWRT configures the system, that I think should be handled in OpenWRT proper. I don't want to maintain two config systems for realtek-poe. That's yet another reason I think these scripts do not belong here.

[Hurricos]

The information in the JSON is derived from /etc/board.d/02_network. That's in OpenWRT proper. That's another reason I think the config should be given by OpenWRT proper.

OK, I see it! I see no reason why /etc/config/poe should not be generated by stock OpenWrt (by e.g. moving 55cd074ca8be60052f5f269f9fcf8439d6282f69's files/etc/uci-defaults/30-realtek-poe script under target/linux/realtek/base-files/etc/uci-defaults).

We'd also need to copy a similar script under target/linux/mpc85xx/base-files/etc/uci-defaults, assuming the BR-200 already has its details in board.json.

In absence of a config file, can we turn on all the ports we detect? This way, you eliminate any dependency on a config.

I want a base config file that the user can mutate and discuss online.

[svanheule]

For reference, the poemgr package ships a static config (for the single supported device), and uses a uci-defaults script to copy that to config/poemgr: https://github.com/blocktrron/poemgr/tree/master/contrib

Do we want to create a poe-common package to provide shared functionality? I think with the proposed changes here, all it would contain is the script to generate a config from board.json. Not including this in file OpenWrt proper (for the realtek target) would save a (teeny) bit of space on devices that don't need it.

[svanheule]

@blocktrron feel free to join the discussion :-)

realtek-poe and poemgr appear to use similar options: "id" vs. "port", and "enabled" vs "disabled".

If we would be able to create a common standard for the config file, that would also open the door for a generic luci-app-poe package that manages the config file and can display the device state (from a standardised poe call).

[j4cbo]

The script fails for me on current openwrt:

root@OpenWrt:~# sh /etc/uci-defaults/30-realtek-poe 
/etc/uci-defaults/30-realtek-poe: line 7: json_init: not found
/etc/uci-defaults/30-realtek-poe: line 8: json_load: not found
/etc/uci-defaults/30-realtek-poe: line 10: json_is_a: not found

Adding . /usr/share/libubox/jshn.sh at the beginning fixes it.

[svanheule]

Changes in last version:

• Change /etc/config/poe umask to 055; we don't need to protect any secrets
• Rework poe_type parsing code. Now in separate function for cleaner modifications
• Add log warning when finding deprecated poe_plus option
• Add missing jshn.sh includes
• Make tests and conditional code in migration script more readable

I've been able to build the daemon, so at least it's compile tested this time. But I can't seem to generate the .ipkg files, so not runtime tested. The scripts should also work.

For now I'm keeping the script with the realtek-poe package, since this is the only consumer of /etc/config/poe. It makes sense to move them elsewhere once there are multiple consumers of this config file, but I think that's premature at this point.

[svanheule]

OK, managed to build the package and install on my GS110TPP. The poe_type directives work as expected, and the port mode changes from "PoE" (poe_type absent) to "PoE+" (poe_type '802.3at'). The port mode is also "PoE" with poe_type '802.3af'.

[svanheule]

@Hurricos @mrnuke do you remain of the opinion that the config file scripts should move out of realtek-poe ASAP, or can this PR continue as-is?

@j4cbo seems to be getting closer to figuring out how to control 802.3bt devices, so the poe_type change would be useful to have.

[Hurricos]

I agree that we should move the config scripts for /etc/config/poe out of realtek-poe and poemgr to a new package poe-common; I haven't written a package in ages, however.

[svanheule]

OK, I'll have a look at creating a poe-common package then. Never created a package before either, and might not get to this week. But it probably won't be more than the one uci-defaults script, so it shouldn't be too hard.

[svanheule]

Please have a look at https://github.com/openwrt/openwrt/pull/14468 for some preparatory changes.

[Hurricos]

OK, I'll have a look at creating a poe-common package then. Never created a package before either, and might not get to this week. But it probably won't be more than the one uci-defaults script, so it shouldn't be too hard.

The poe-common package proposal can be found here: https://lists.openwrt.org/pipermail/openwrt-devel/2024-January/042155.html. It just hasn't been merged.

[howels]

@svanheule is there any progress on the changes proposed in your suggestion for a poe-common package Would be helpful if we have a way to enforce defaults especially on higher port-count PoE switches.

I am happy to test and provide feedback but have been tracking work here on GitHub instead of in the mailing lists.

[Neustradamus]

To follow this PR :)