Meter mode code refactor

[Explorer09]

The main goal of this set of patches is to remove the use of CUSTOM_METERMODE in defaultMode values, and to adjust Meter_setMode() code in preparation of the supportedModes feature (PR #1387).

[BenBE]

Any reason, why you dropped 2538896 from this series?

[Explorer09]

@BenBE It's better for 2538896 to merge with your PR and that change is slightly irrelevant to the other parts of code refactoring here. Besides the function name nextSupportedMode would make less sense until the real supportedMode field is implemented.

[BenBE]

Sure, just noticed when comparing your branches.

LGTM.

[Explorer09]

More code refactoring, and thus it's good to re-review this.

• Make Meter_modes array private to Meter.c
• Define type MeterModeId to uint8_t and make a separate header for it. (If I put that typedef in Meter.h it could cause a header dependency hell that I had a hard time resolving it.)

[BenBE]

• In e5bcc4f there's unrelated changes in Header.c.
• In e5bcc4f for MeterMode.h do typedef int MeterModeId; (saving on including stdint.h and several casts)
• In e5bcc4f for Settings.c inside Settings_defaultMeters are unrelated changes
• In dab8ebf in MeterMode.h consider typedef enum MeterModeId_ { … } MeterModeId;
• In dab8ebf in MeterMode.h there should be two blank lines before declarations
• In 2461e6e changes for code structure and functional changes are mixed

[Explorer09]

@BenBE

• In dab8ebf in MeterMode.h there should be two blank lines before declarations

Will do.

• In e5bcc4f there's unrelated changes in Header.c.
• In 2461e6e changes for code structure and functional changes are mixed
• In e5bcc4f for Settings.c inside Settings_defaultMeters are unrelated changes

All of these are style changes being mixed with changes in behavior. Do you mean to drop the style changes or split them into seperate commits?

• In e5bcc4f for MeterMode.h do typedef int MeterModeId; (saving on including stdint.h and several casts)
• In dab8ebf in MeterMode.h consider typedef enum MeterModeId_ { … } MeterModeId;

I think you're interested in some size reports:

gcc-11 with options -O2 -flto -Wl,--as-needed

    text    data     bss     dec    hex
  359500   26248    6520  392268  5fc4c (a) MeterModeId in uint8_t
  359460   26248    6520  392228  5fc24 (b) MeterModeId in signed int
  359452   26248    6520  392220  5fc1c (c) MeterModeId in unsigned int
  359500   26248    6520  392268  5fc4c (d) uint8_t MeterModeId with reorder
                                            of Meter & MeterClass structures

gcc-11 with options -Os -flto -Wl,--as-needed

    text    data     bss     dec    hex
  282899   26320    6552  315771  4d17b (a) MeterModeId in uint8_t
  282867   26320    6552  315739  4d15b (b) MeterModeId in signed int
  282865   26320    6552  315737  4d159 (c) MeterModeId in unsigned int
  282899   26320    6552  315771  4d17b (d) uint8_t MeterModeId with reorder 
                                            of Meter & MeterClass structures

clang-16 with options -Os -flto -Wl,--as-needed

    text    data     bss     dec    hex
  291703   25380    6368  323451  4ef7b (a) MeterModeId in uint8_t
  291693   25380    6368  323441  4ef71 (b) MeterModeId in signed int
  291692   25380    6368  323440  4ef70 (c) MeterModeId in unsigned int
  291703   25004    6368  323075  4ee03 (d) uint8_t MeterModeId with reorder
                                            of Meter & MeterClass structures

I gathered these from GitHub Actions CI. And you are right in that using uint8_t for MeterModeId doesn't help with the code size, but interestingly making MeterModeId an unsigned int (the (c) case) can reduce a few bytes of the compiled binary.
The (d) case is interesting and I didn't figure out the cause yet (I moved the mode member to close to the maxItems and curItems members of the MeterClass and Meter structures, thus kills the unnecessary padding). I think I'll pick the (c) case.

[Explorer09]

• In 2461e6e changes for code structure and functional changes are mixed

I'm a little confused what you mean by "code structure and functional changes are mixed" and how should I split the commits.

[BenBE]

@BenBE

• In e5bcc4f there's unrelated changes in Header.c.
• In 2461e6e changes for code structure and functional changes are mixed
• In e5bcc4f for Settings.c inside Settings_defaultMeters are unrelated changes

All of these are style changes being mixed with changes in behavior. Do you mean to drop the style changes or split them into seperate commits?

In the first commit, there's this sizeof change in the commit, that's not purely code formatting, but a distinct change (although no functional difference). Similar goes for the other changes I listed. Just split those changes (you can merge the two sizeof changes in one commit afterwards).

• In e5bcc4f for MeterMode.h do typedef int MeterModeId; (saving on including stdint.h and several casts)
• In dab8ebf in MeterMode.h consider typedef enum MeterModeId_ { … } MeterModeId;

I think you're interested in some size reports:

gcc-11 with options -O2 -flto -Wl,--as-needed

    text    data     bss     dec    hex
  359500   26248    6520  392268  5fc4c (a) MeterModeId in uint8_t
  359460   26248    6520  392228  5fc24 (b) MeterModeId in signed int
  359452   26248    6520  392220  5fc1c (c) MeterModeId in unsigned int
  359500   26248    6520  392268  5fc4c (d) uint8_t MeterModeId with reorder
                                            of Meter & MeterClass structures

gcc-11 with options -Os -flto -Wl,--as-needed

    text    data     bss     dec    hex
  282899   26320    6552  315771  4d17b (a) MeterModeId in uint8_t
  282867   26320    6552  315739  4d15b (b) MeterModeId in signed int
  282865   26320    6552  315737  4d159 (c) MeterModeId in unsigned int
  282899   26320    6552  315771  4d17b (d) uint8_t MeterModeId with reorder 
                                            of Meter & MeterClass structures

clang-16 with options -Os -flto -Wl,--as-needed

    text    data     bss     dec    hex
  291703   25380    6368  323451  4ef7b (a) MeterModeId in uint8_t
  291693   25380    6368  323441  4ef71 (b) MeterModeId in signed int
  291692   25380    6368  323440  4ef70 (c) MeterModeId in unsigned int
  291703   25004    6368  323075  4ee03 (d) uint8_t MeterModeId with reorder
                                            of Meter & MeterClass structures

I gathered these from GitHub Actions CI. And you are right in that using uint8_t for MeterModeId doesn't help with the code size, but interestingly making MeterModeId an unsigned int (the (c) case) can reduce a few bytes of the compiled binary. The (d) case is interesting and I didn't figure out the cause yet (I moved the mode member to close to the maxItems and curItems members of the MeterClass and Meter structures, thus kills the unnecessary padding). I think I'll pick the (c) case.

(b) and (c) are fine. Basically we have (b) right now. Be sure to update the format strings accordingly and you'll be able to skip that one cast.

[BenBE]

LGTM.