fightwarn - fix how we work with enums #907
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
Note: everything not covered before this PR got aliased into default cases. I do not know the hardware and models to rule this out otherwise, possibly assignments of binary and/or smart protocol (lack of?) support are now wrong.
…ge, especially when for no reason other than to have an un-used initial value
jimklimov
commented
Nov 26, 2020
| case TRIPP_LITE_SMART_0004: | ||
| case TRIPP_LITE_OMNIVS: | ||
| case TRIPP_LITE_OMNIVS_2001: | ||
| case TRIPP_LITE_UNKNOWN: | ||
| default: |
There was a problem hiding this comment.
Input from Tripplite HW/spec specialists would be welcome here...
jimklimov
commented
Nov 26, 2020
| @@ -222,6 +227,9 @@ static int is_smart_protocol() | |||
| case TRIPP_LITE_SMART_0004: | |||
| case TRIPP_LITE_SMART_3005: | |||
| return 1; | |||
| case TRIPP_LITE_OMNIVS: | |||
| case TRIPP_LITE_OMNIVS_2001: | |||
| case TRIPP_LITE_UNKNOWN: | |||
| default: | |||
There was a problem hiding this comment.
Input from Tripplite HW/spec specialists would be welcome here...
jimklimov
commented
Nov 26, 2020
| @@ -777,6 +785,9 @@ static int control_outlet(int outlet_id, int state) | |||
| #pragma GCC diagnostic pop | |||
| #endif | |||
|
|
|||
| case TRIPP_LITE_OMNIVS: | |||
| case TRIPP_LITE_OMNIVS_2001: | |||
| case TRIPP_LITE_UNKNOWN: | |||
| default: | |||
| upslogx(LOG_ERR, "control_outlet unimplemented for protocol %04x", tl_model); | |||
There was a problem hiding this comment.
Input from Tripplite HW/spec specialists would be welcome here...
|
Merging as a little-impacting change at this time, but hoping Tripplite experts in particular would revise this recent state of codebase vs. actual hardware models. |
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.
Follows from efforts at #823 / #844 to squash as many bugs and "fishy coding" warnings found by newest compilers.
This PR is separated from the stream of others since it addresses a common theme - that a few
switch()cases reacting to values of anenumdid not cover all options listed in that enum - so something might end up not handled intentionally and just fell through default handling.In case of
powerpdriver below, such change is trivial - there were two options in enum, so one handled already and other "obviously" same asdefault.The case of
tripplite_usbworries me more, since there are 6 options and just a small subset of these were handled initially, to state support of binary or smart protocol, and to control outlets. While the change to spell out the previously not listed options as aliases intodefaultcase does not change the practical outcome compared to older builds, a domain expert would be beneficial to check the actual support per model/protocol and maybe shuffle the lines to match actual hardware capabilities correctly.Comments in code imply there might be more options in reality (e.g. a "Add model 3004?"), revising and adding those to the driver would also be nice.
More so that it seems Tripplite support is a focus point enough that a vendor representative is active in the NUT mailing list.
One case was also of needlessly pre-assigning an invalid out-of-range value (that we did not use anyway, reassigned in code below that to functions' returns first).
Merging this PR should not change NUT behavior compared to existing codebase; it was exposed to focus attention of experts who might update the drivers in future PRs.