-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
|
3e4cfd1
to
33b1595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality wise seems ok, but a few cleanups can be done before merging.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is adding new functionality as part of another unrelated commit (the cmake migraiton), consider splitting in two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be !defined || !defined (that is, only use the default path below if both are defined). Possibly expressed as !(defined && defined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error handling on bad parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit wrong when "core" functionality includes parts from tools.. other way around would feel less incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
33b1595
to
b92f1ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now
No description provided.