Hurricos / realtek-poe

5 stars 10 forks source link

realtek-poe can't make use of dynamically fetched port_count because it configures ports first #12

Closed Hurricos closed 2 years ago

Hurricos commented 2 years ago

This issue is explicitly mentioned by @mrnuke here.

You can see the effect of this on e.g. ubus call poe info after first start here.

Here is the output of realtek-poe -d demonstrating this: https://paste.c-net.org/LockupDebate

[mkennedy@fedora ~]$ grep '> 1a .. ' realtek-poe-d.log 
TX -> 1a 06 00 02 ff ff ff ff ff ff ff 1b
TX -> 1a 0d 01 02 ff ff ff ff ff ff ff 23
:
TX -> 1a 30 06 02 ff ff ff ff ff ff ff 4b
TX -> 1a 37 07 02 ff ff ff ff ff ff ff 53
# `realtek-poe` is only sending config for ports 1-8 initially.
# A `ubus call poe reload` is performed, and ...
:
TX -> 1a 64 00 02 ff ff ff ff ff ff ff 79
TX -> 1a 6b 01 02 ff ff ff ff ff ff ff 81
:
TX -> 1a fe 16 02 ff ff ff ff ff ff ff 29
TX -> 1a 05 17 02 ff ff ff ff ff ff ff 31
# ... suddenly `realtek-poe` is sending configuration frames for ports 1-24.
mrnuke commented 2 years ago

There's an app for that: #8

Hurricos commented 2 years ago

OK, that's really odd. I didn't notice that PR, but I did manage to come up with a patch --


diff --git a/src/main.c b/src/main.c
index 0310f67..d09c55e 100644
--- a/src/main.c
+++ b/src/main.c
@@ -126,15 +126,19 @@ static void load_port_config(struct uci_context *uci, struct uci_section *s)

 static void load_global_config(struct uci_context *uci, struct uci_section *s)
 {
-       const char *budget, *guardband;
+       const char *budget, *guardband, *port_count;

        budget = uci_lookup_option_string(uci, s, "budget");
        guardband = uci_lookup_option_string(uci, s, "guard");
+       port_count = uci_lookup_option_string(uci, s, "port_count");

        config.budget = budget ? strtof(budget, NULL) : 31.0;
        config.budget_guard = config.budget / 10;
        if (guardband)
                config.budget_guard = strtof(guardband, NULL);
+       config.port_count = port_count ? strtoul(port_count, NULL, 0) : 8;
+       if (config.port_count > MAX_PORT)
+               config.port_count = MAX_PORT;
 }

 static void
 ``
mrnuke commented 2 years ago

"port_count" is redundant if you have a bunch of ports,each with a unique, monotonically increasing ID.

Hurricos commented 2 years ago

"port_count" is redundant if you have a bunch of ports,each with a unique, monotonically increasing ID.

Yep, I can see how just pulling the max might be better.

mrnuke commented 2 years ago

I won't claim my approach is better. That's in the eye of the beer-holder. I will only claim it's a lower SLOC count and doesn't need modifications to the config file :stuck_out_tongue:

Hurricos commented 2 years ago

With #8 merged, this is fixed.