Skip to content
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

Add grisp pack command #85

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Add grisp pack command #85

merged 2 commits into from
Sep 6, 2024

Conversation

sylane
Copy link
Contributor

@sylane sylane commented Sep 5, 2024

@sylane
Copy link
Contributor Author

sylane commented Sep 5, 2024

The dependency will be fix when grisp_tools is published

@sylane sylane force-pushed the sylane/pack-command branch 2 times, most recently from 03de0c9 to 04fa32b Compare September 5, 2024 18:28
CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- New `-t/--tar` option to the deploy command to save a grisp release tarball in
the `_grisp/deploy` directory.
- New firmware command to generate GRiSP 2 binary firmwares: [#83](https://github.com/grisp/rebar3_grisp/pull/83)
- New pack command to generate GRiSP 2 software update package: [#84](https://github.com/grisp/rebar3_grisp/pull/84)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong link should be PR 85

PackageFile = grisp_tools_util:maybe_relative(PackageFile0, ?MAX_DDOT),
iolist_to_binary([
"Instructions to update GRiSP2 software on the board with grisp_updater:\n",
" NOTE: the software running on the board MUST run grisp_updater by adding the grisp_updater_grisp2 dependency.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to publish grisp_updater_grisp2 on github, and explain this in the README at least, we could also move advanced topics in a separate markdown to reduce the clutter to first time visitors of the github repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is explained in the README.md now.

"If no firmware file is specified, it will be generated by "
"calling 'rebar3 grisp firmware' with the optional release name "
"and version. As for the firmware command, options passed after "
"'--' are sent to the rebar 3 release task.\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As last thing we should document the purpose of this command in a markdown file at least, I would move it in ADVANCED.md toghether with the formware command explanation to remove extra clutter from the main README. I would just mention these and link the secondary document.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have a point by point tutorial in the GRiSP wiki

Plus, I would add a simple example for the command in the README of rebar3_grisp and then put a link to any advanced documentation there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a wiki tutorial would be nice, and some documentation about A/B software update in general. Not part of this PR though.

src/rebar3_grisp_pack.erl Show resolved Hide resolved
Comment on lines +40 to +42
{force_firmware, $F, "force-firmware", {boolean, false}, "Force firmware building even if it already exists"},
{force, $f, "force", {boolean, false}, "Replace existing files"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the 'force' command should also force the firmware building

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was at first, but then it mean that if the file exists you are forces to generate a firmware...
I could change it back to have only one force option, in this command and in firmware too, that is similar.

"If no firmware file is specified, it will be generated by "
"calling 'rebar3 grisp firmware' with the optional release name "
"and version. As for the firmware command, options passed after "
"'--' are sent to the rebar 3 release task.\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have a point by point tutorial in the GRiSP wiki

Plus, I would add a simple example for the command in the README of rebar3_grisp and then put a link to any advanced documentation there

PackageFile = grisp_tools_util:maybe_relative(PackageFile0, ?MAX_DDOT),
iolist_to_binary([
"Instructions to update GRiSP2 software on the board with grisp_updater:\n",
" NOTE: the software running on the board MUST run grisp_updater by adding the grisp_updater_grisp2 dependency.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, we should probably add the option to include grisp_updater when we run rebar3 grisp configure. But this an additional feature and can be done in a future PR

Copy link
Member

@ziopio ziopio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff

@sylane sylane merged commit a56e7b6 into master Sep 6, 2024
10 checks passed
@sylane sylane deleted the sylane/pack-command branch September 6, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants