Pbtools update #24
Pbtools update #24
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 68.06% 68.15% +0.09%
==========================================
Files 54 54
Lines 2787 2795 +8
==========================================
+ Hits 1897 1905 +8
Misses 890 890
|
jonasblixt
force-pushed
the
pbtools-update
branch
4 times, most recently
from
3e4cfd1
to
33b1595
Compare
| @@ -245,9 +243,11 @@ bool pbstate_is_system_active(pbstate_system_t system) | |||
| return config.enable == PB_STATE_A_ENABLED; | |||
| case PBSTATE_SYSTEM_B: | |||
| return config.enable == PB_STATE_B_ENABLED; | |||
| case PBSTATE_SYSTEM_NONE: | |||
There was a problem hiding this comment.
This is adding new functionality as part of another unrelated commit (the cmake migraiton), consider splitting in two.
tools/pbstate/src/main.c
Outdated
| @@ -134,10 +134,15 @@ int main(int argc, char * const argv[]) | |||
| } | |||
| } | |||
|
|
|||
| #if !defined(PRIMARY_PART) && !defined(BACKUP_PART) | |||
There was a problem hiding this comment.
I think this should be !defined || !defined (that is, only use the default path below if both are defined). Possibly expressed as !(defined && defined).
tools/pbstate/src/pbstate.c
Outdated
|
|
||
| int pbstate_read_board_reg(unsigned int index, uint32_t *value) | ||
| { | ||
| if (index > (NO_OF_BOARD_REGS - 1)) |
There was a problem hiding this comment.
To allow for (easier) later extension with more board_regs, I would at least consider numbering them with index == 0 as the last register, and then count backwards.
tools/pbstate/src/main.c
Outdated
| @@ -116,6 +136,18 @@ int main(int argc, char * const argv[]) | |||
| case 'i': | |||
| flag_show = true; | |||
| break; | |||
| case 'w': | |||
| flag_write_board_reg = true; | |||
| board_reg_index = strtol(optarg, NULL, 0); | |||
There was a problem hiding this comment.
No error handling on bad parsing?
|
|
||
| if (read_config(fd, &config_backup) != 0) { | ||
| LOG("Could not read backup config data\n"); | ||
| if (fd != -1) { |
There was a problem hiding this comment.
As a general style I prefer (and most code seems to be) in a style where the success case is as little indented as possible, that is you try as much as possible to have
if (error) {
.. return or break
}
<success case>
If nothing else just to keep consistency, consider changing.
There was a problem hiding this comment.
Agreed, but in this case the function should not return on error but try to load/read the backup. I'll keep it as is.
There was a problem hiding this comment.
Ah, that is my misunderstanding of the logic in the method.
tools/pbstate/src/pbstate.c
Outdated
| memcpy(&config, &config_backup, sizeof(config)); | ||
| } else if (!primary_ok) { | ||
| LOG("Primary configuration corrupt\n"); | ||
| LOG("Backup configuration corrupt\n"); | ||
| LOG("Both primary and backup state corrupt\n"); | ||
| err = -1; |
There was a problem hiding this comment.
Earlier in the method it returns an -errno, however here it is -1 (which maps ot EPERM, which is probably not what should be returned). Within a single method, either just have 0/-1, or 0/-errno, but don't mix
src/boot/ab_state.c
Outdated
| } __attribute__((packed)); | ||
|
|
||
| // For 'struct pb_boot_state' | ||
| #include "../tools/pbstate/src/pbstate.h" |
There was a problem hiding this comment.
Seems a bit wrong when "core" functionality includes parts from tools.. other way around would feel less incorrect
.github/workflows/build.yml
Outdated
| @@ -34,8 +34,7 @@ jobs: | |||
| - name: Build pbstate | |||
| run: | | |||
| pushd tools/pbstate | |||
| autoreconf -fi | |||
| ./configure | |||
| cmake . | |||
There was a problem hiding this comment.
CMake generally don't like at all to build in same directory (which is probably why CI fails, I haven't checked).
Fix -Wextra/-pedantic warnings
This adds cmake option for specifying a default for primary and backup partition through the parameters: - PB_STATE_PRIMARY_PART - PB_STATE_BACKUP_PART This is only relevant for the CLI and does not change any behaviour with the library portion.
This adds the additional "board registers". The idéa is to have a few board specific registers to control custom board specific logic. These registers can be accessed in the board bootloader code and through the pbstate library and CLI. This commit also moves the struct to a public header shared with the bootloader code.
Use errno whenever possible.
Apparently, this is now required.
The same crc routine is used in the main code, let's use that instead.
jonasblixt
force-pushed
the
pbtools-update
branch
from
33b1595
to
b92f1ad
Compare
No description provided.