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: use SMF #70787

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

Conversation

maass-hamburg
Copy link
Collaborator

@maass-hamburg maass-hamburg commented Mar 27, 2024

Use the State Machine Framework for hawkBit. Includes a lot of cleanup of the hawkBit subsystem.

Resolves: #71576
Resolves: #71691
Resolves: #71759

Depends on/the following PRs should be merged before this:

the commits from before mgmt: hawkbit: use SMF are from #71037

this is a WIP

@maass-hamburg maass-hamburg force-pushed the hawkbit_less_global branch 7 times, most recently from 49080dc to 80fa584 Compare April 8, 2024 11:26
@maass-hamburg maass-hamburg changed the title mgmt: hawkbit: make hb_context non global mgmt: hawkbit: use SMF Apr 8, 2024
@maass-hamburg maass-hamburg force-pushed the hawkbit_less_global branch 3 times, most recently from d1a50eb to 4ae2630 Compare April 16, 2024 09:25
@kartben
Copy link
Collaborator

kartben commented Apr 17, 2024

cc @glenn-andrews

@maass-hamburg maass-hamburg force-pushed the hawkbit_less_global branch 2 times, most recently from d01cb3b to 93104c8 Compare April 18, 2024 11:41
Copy link
Contributor

@glenn-andrews glenn-andrews left a comment

Choose a reason for hiding this comment

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

Consider transitioning to an error-handling state rather than using smf_set_state(SMF_CTX(s), NULL);

subsys/mgmt/hawkbit/hawkbit.c Outdated Show resolved Hide resolved
@real-tintin
Copy link

This is great! We have considered rewriting the implementation to use a state machine due to multiple issue (I previously wrote in the roadmap issue). As I wrote there, we have made adaptions and bug fixes e.g., issues with double freeing and handling of NULL response (crashes hard during download if not checked). But I see that you have improved on the malloc and I strongly believe that the SMF will solve a bunch of issues itself.

I would like to give this a try. I will merge (locally for testing) our adaptions e.g., a dynamic config e.g., setting endpoints and tokens (during a device provisioning over BLE) - I saw that it already exist a PR for a similar change.

@maass-hamburg
Copy link
Collaborator Author

maass-hamburg commented Apr 18, 2024

This is great! We have considered rewriting the implementation to use a state machine due to multiple issue (I previously wrote in the roadmap issue). As I wrote there, we have made adaptions and bug fixes e.g., issues with double freeing and handling of NULL response (crashes hard during download if not checked). But I see that you have improved on the malloc and I strongly believe that the SMF will solve a bunch of issues itself.

I would like to give this a try. I will merge (locally for testing) our adaptions e.g., a dynamic config e.g., setting endpoints and tokens (during a device provisioning over BLE) - I saw that it already exist a PR for a similar change.

Thanks for the feedback!
If you find bugs, please feel free to open an issue. And if you have a fix for it, it would be wonderful if you contribute it back into the zephyr main. It makes thinks easier for all of us.

The same is true for new features, feel free to contribute. Just look at the features we added to hawkBit in the last weeks: here

free(hb_context.response_data);
k_sem_give(&probe_sem);
return hb_context.code_status;
smf_set_terminate(SMF_CTX(s), s->hb_context.code_status);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this ever be zero? e.g. with a successful download?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good from the SMF side. I can't answer to the functionality of the driver.

Does this look correct as a state diagram:

hawkbit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it does, but not including today's changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it does, but not including today's changes.

Heh. okay.

@maass-hamburg maass-hamburg force-pushed the hawkbit_less_global branch 5 times, most recently from c78cf8d to 2a5891a Compare April 22, 2024 09:44
@real-tintin
Copy link

Tested the recent changes, works fine! I guess I don't have the buttons to approve this...

Awesome work, feel much more rigid now! 🙂

@real-tintin
Copy link

Integrated latest changes and tested them, this includes:

  • No update
  • Cancel of update
  • Download and image swap

No issues.

We have a setup where devices are OTA updated every X h, stress test. I will create a alpha release with your latest changes and run in the setup. I can get back to you with the result after the weekend.

@maass-hamburg
Copy link
Collaborator Author

Integrated latest changes and tested them, this includes:

  • No update
  • Cancel of update
  • Download and image swap

No issues.

We have a setup where devices are OTA updated every X h, stress test. I will create a alpha release with your latest changes and run in the setup. I can get back to you with the result after the weekend.

thats sounds good

@maass-hamburg maass-hamburg force-pushed the hawkbit_less_global branch 3 times, most recently from 93ba8d5 to 49c95fc Compare April 29, 2024 07:28
staticly init hawkbit_work_handle

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
add option for autohandler to only run once.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
@real-tintin
Copy link

The devices have OTA updated as expected over the weekend. I saw that you pushed some changes today. Should these affect the SMF?

@maass-hamburg
Copy link
Collaborator Author

maass-hamburg commented Apr 29, 2024

The devices have OTA updated as expected over the weekend. I saw that you pushed some changes today. Should these affect the SMF?

It shouldn't. See them here: https://github.com/zephyrproject-rtos/zephyr/compare/cd2578014f1965de9747b82ac29cc929504ee273..93ba8d566c509b1b8b6dd383d751843ae10386f2
and changes from rebaising to #71037

Copy link

@real-tintin real-tintin left a 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. I have performed integration testing - multiple devices getting OTA updated every 1 h, over one weekend.

Add hawkbit_autohandler_wait() to be able
to wait for the autohandler to finish.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Also use a workqueue, when execution of
hawkBit is requested via shell.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
If the run of the autohandler is started from shell,
it will be logged.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Be able to delay the next run of
the hawkbit autohandler.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Mention change of hawkbit autohandler and shell in the
migration guide and the release notes.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
use smf for hawkbit.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
remove unused hawkbit responses.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
allow the use of other tenants.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
rename close to cancel, as it is more fitting.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Be able to change the tls certicicate tag.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
@maass-hamburg maass-hamburg added DNM This PR should not be merged (Do Not Merge) area: hawkBit labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: hawkBit DNM This PR should not be merged (Do Not Merge)
Projects
None yet
4 participants