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

mgmt: hawkbit: autohandler and shell optimize #71037

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

maass-hamburg
Copy link
Collaborator

@maass-hamburg maass-hamburg commented Apr 3, 2024

also use the workqueue for the run of hawkBit, when requested via the shell. This way the run via the shell will use the same thread as the autohandler run. We also can eliminate redundant code.

This has mainly the benefit of not having to extend the stack size of the shell threads. Explicitly when having multiple shell instances, then the stack of every instance has to be bigger.

This is needed for #70787 as it moves the buffers from global to the stack.

include/zephyr/mgmt/hawkbit.h Outdated Show resolved Hide resolved
subsys/mgmt/hawkbit/hawkbit.c Outdated Show resolved Hide resolved
subsys/mgmt/hawkbit/shell.c Outdated Show resolved Hide resolved
@zephyrbot zephyrbot added the area: Samples Samples label Apr 10, 2024
@zephyrbot zephyrbot requested a review from nashif April 10, 2024 10:26
@maass-hamburg maass-hamburg force-pushed the hawkbit_shell_optimize branch 4 times, most recently from 3a04bdf to d7fcf8c Compare April 10, 2024 11:06
subsys/mgmt/hawkbit/hawkbit.c Outdated Show resolved Hide resolved
subsys/mgmt/hawkbit/shell.c Show resolved Hide resolved
@maass-hamburg maass-hamburg force-pushed the hawkbit_shell_optimize branch 2 times, most recently from 01155a7 to 12df207 Compare April 19, 2024 16:44
subsys/mgmt/hawkbit/hawkbit.c Outdated Show resolved Hide resolved
subsys/mgmt/hawkbit/shell.c Show resolved Hide resolved
include/zephyr/mgmt/hawkbit.h Outdated Show resolved Hide resolved
include/zephyr/mgmt/hawkbit.h Outdated Show resolved Hide resolved
include/zephyr/mgmt/hawkbit.h Outdated Show resolved Hide resolved
@aescolar aescolar added this to the v4.0.0 milestone Jun 17, 2024
@aescolar
Copy link
Member

Bulk changing the milestone from everything which did not make it to the v3.7.0 freeze deadline
and which is not labeled as a bug or documentation (or does not appear clearly as only containing
a bug fix or a documentation change).
If you disagree about this change please add the milestone again.

Please note that until rc2 only PRs containing bug fixes, docs and tests can be merged.
After that and until release only bug fixes and docs changes.

Exceptions require approval by the release team and a vote by the TSC.
https://docs.zephyrproject.org/latest/project/release_process.html#stabilization-phase

@maass-hamburg maass-hamburg force-pushed the hawkbit_shell_optimize branch 2 times, most recently from 18b454b to c6cc5a6 Compare August 1, 2024 06:24
@maass-hamburg
Copy link
Collaborator Author

I like to get forward with this PR.
First of all as a Maintainer I want to say that:

  • the autohandler and the hawkBit shell will stay.
  • useful additions to both of them are welcome.

Both the shell and the autohandler are a great at making hawkBit easy to use and accessible.
Not everybody want to write their own autohandler and therefore use the autohandler, which will just work and you don't have to do much yourself. And fore the ones, who don't want to use this autohandler, they can, if this PR gets merged, deactivate the included autohandler.

That being said, I hope we can get this PR approved and merged soon, so we can finally be able to get to #70787

@maass-hamburg maass-hamburg added the dev-review To be discussed in dev-review meeting label Aug 20, 2024
@MaureenHelm MaureenHelm removed the dev-review To be discussed in dev-review meeting label Aug 22, 2024
@maass-hamburg
Copy link
Collaborator Author

rebased to resolve conflicts

@maass-hamburg
Copy link
Collaborator Author

@ycsin are you coming back for a review?
In the dev-review meeting in the last week there where no objections against my changes.

staticly init hawkbit_work_handle

Signed-off-by: Fin Maaß <[email protected]>
add option for autohandler to only run once.

Signed-off-by: Fin Maaß <[email protected]>
Add hawkbit_autohandler_wait() to be able
to wait for the autohandler to finish.

Signed-off-by: Fin Maaß <[email protected]>
Also use a workqueue, when execution of
hawkBit is requested via shell.

Signed-off-by: Fin Maaß <[email protected]>
If the run of the autohandler is started from shell,
it will be logged.

Signed-off-by: Fin Maaß <[email protected]>
Be able to delay the next run of
the hawkbit autohandler.

Signed-off-by: Fin Maaß <[email protected]>
Mention change of hawkbit autohandler and shell in the
migration guide and the release notes.

Signed-off-by: Fin Maaß <[email protected]>
Add function to get the poll interval.
This is needed to seperate the autohandler
from the main hawkbit code.

Signed-off-by: Fin Maaß <[email protected]>
seperate the autohandler from the main
hawkbit source. This way the autohandler can
be disabled if it is not needed.

Signed-off-by: Fin Maaß <[email protected]>
seperate the hawkbit header files, to make
it clearer.

Signed-off-by: Fin Maaß <[email protected]>
move HAWKBIT_JSON_URL out of the header.

Signed-off-by: Fin Maaß <[email protected]>
Mention change of the hawkbit header files.

Signed-off-by: Fin Maaß <[email protected]>
@maass-hamburg
Copy link
Collaborator Author

rebased due to unrelated fails in the ci

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

@ycsin are you coming back for a review?

Had another look but there seems to be nothing new to review, I'd say my previous review is still up-to-date

First of all as a Maintainer I want to say that:

  • the autohandler and the hawkBit shell will stay.
  • useful additions to both of them are welcome.

Both the shell and the autohandler are a great at making hawkBit easy to use and accessible. Not everybody want to write their own autohandler and therefore use the autohandler, which will just work and you don't have to do much yourself. And fore the ones, who don't want to use this autohandler, they can, if this PR gets merged, deactivate the included autohandler.

I agree that the bundled autohandler in this PR might be useful, I have no qualms if this PR updated the one in the samples to make it more portable, which IMHO serves pretty much the same purpose to show the users how to build one, instead of forcing an implementation on them.

I've had my custom implementation of the autohandler in the past, had I chose to upstream and developed it around my use case, folks that came after me probably have to either use it my way or go away, I think that would make hawkBit less inviting

Comment on lines +36 to +46
config HAWKBIT_AUTOHANDLER
bool "hawkBit autohandler"
select EVENTS
default y
help
Activate autohandler to handle the update process automatically.

config HAWKBIT_SHELL
bool "hawkBit shell utilities"
depends on SHELL
depends on HAWKBIT_AUTOHANDLER
Copy link
Member

Choose a reason for hiding this comment

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

And fore the ones, who don't want to use this autohandler, they can, if this PR gets merged, deactivate the included autohandler.

Does this dependency means that for user who doesn't want to use the bundled autohandler, they lose access to shell for updating the configurations as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well yes at least when they don't activate the Kconfig for the autohandler. They could still choose to include the autohandler and don't use it theirself.

@maass-hamburg
Copy link
Collaborator Author

@ycsin are you coming back for a review?

Had another look but there seems to be nothing new to review, I'd say my previous review is still up-to-date

First of all as a Maintainer I want to say that:

  • the autohandler and the hawkBit shell will stay.
  • useful additions to both of them are welcome.

Both the shell and the autohandler are a great at making hawkBit easy to use and accessible. Not everybody want to write their own autohandler and therefore use the autohandler, which will just work and you don't have to do much yourself. And fore the ones, who don't want to use this autohandler, they can, if this PR gets merged, deactivate the included autohandler.

I agree that the bundled autohandler in this PR might be useful, I have no qualms if this PR updated the one in the samples to make it more portable, which IMHO serves pretty much the same purpose to show the users how to build one, instead of forcing an implementation on them.

I've had my custom implementation of the autohandler in the past, had I chose to upstream and developed it around my use case, folks that came after me probably have to either use it my way or go away, I think that would make hawkBit less inviting

@ycsin As I said before the autohandler and the shell are going to stay and if there are good reasons, like here, they are also going to be extended. Requested changes in the other way are hereby dismissed. You can follow the escalation process if you are not okay with that, otherwise I expect a review approving this or requesting changes that are not on this fundamental level.

@maass-hamburg maass-hamburg dismissed ycsin’s stale review September 11, 2024 10:58

see coments above, he also removed himself from the colaborators of hawkbit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: hawkBit area: Process area: Samples Samples Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants