Cleanup code madness, round 1 #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Having the "0x" prefix repeatedly in a dump of numbers that's obviously hexadecimal is pointless. Even hexdump's canonical format does not have the prefix. Don't print the prefix in packet dumps. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
The "state_timeout" and ubus "conn" variables don't need to be scoped globaly. The use of "state_timeout" in state_timeout_cb() is not problematic. A pointer to "state_timeout" is passed in through the "t" parameter. Move these two declarations within main(), and use designated initializers to initialize the data structures. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
The resolution is 0.1 W in botth the PoE spec and the MCU protocol. To enable this resolution, store these configs as floating point. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Using an enum to index into a config array, to deserialize a blob which was serialized from a config decoded by uci reminds me of Inception. While the movie was good, none of the characters were software engineers -- and for good reason. This level of indirection works better for hollywood. For reading small uci config sections, just use uci_lookup_option_string() directly. The proof is in the pudding, and more than half of the config parsing code vanishes. The "budged" and "guard" configs are now parsed as floating point, enabling 0.1 W resolution. Also, use a more conservative 31.0 W as the default budget if one isnot given in the uci config. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
The "sendframe" command only has one argument. It's either there, or it aint't. It's a binary proposition. While enums are innocent, their us in this situation is not. Stop abusing enums! Instead, drop the POE_SENDFRAME_ enum, and dereference the blob_attr directly. This reduces about 4 tons of code bloat. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Hurricos
reviewed
Jun 26, 2022
| @@ -391,7 +391,7 @@ poe_cmd_power_mgmt_mode(unsigned char mode) | |||
|
|
|||
| /* 0x18 - Set global power budget */ | |||
| static int | |||
| poe_cmd_global_power_budget(int budget, int guard) | |||
| poe_cmd_global_power_budget(float budget, float guard) | |||
There was a problem hiding this comment.
write16_be expects a uint16_t, yet this change causes it to now be passed a float, twice (float * int = float).
I'd ask you to cast, but I'm more alarmed by the fact that this code compiles and works.
There was a problem hiding this comment.
There's an implicit int conversion. That is valid C.
Now there is an issue if the result is outside of [0, UINT16_MAX]. But that issue also existed before this patch,
There was a problem hiding this comment.
Here's the wording from the C standard, for those playing along at home:
6.3.1.4 Real floating and integer
1 When a finite value of real floating type is converted to an integer type other than _Bool,
the fractional part is discarded (i.e., the v alue is truncated to ward zero). If the v alue of
the integral part cannot be represented by the integer type, the behavior is undefined.
| @@ -89,99 +89,50 @@ static void write16_be(uint8_t *raw, uint16_t value) | |||
| raw[1] = value & 0xff; | |||
| } | |||
|
|
|||
| static void | |||
There was a problem hiding this comment.
I think I've seen this pattern elsewhere in OpenWrt uci parsing code -- maybe it exists to standardize it and make it machine-readable? -- but --
- I might be wrong and this part of
realtek-poemight just be a blogicism - Even if I found this pattern in other uci / ubus parsing code, I don't think I'd be able to call up the right people to explain their patterns.
So this has my blessing.
|
Feedback given. Tweak to cast those floats before |
|
I am comfortable with this; the lack of WIP suggests you are as well. Merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main point of this PR is to eliminate ~50 lines of code while maintaining functionality.