Skip to content

lib: adding support for modem to updatehub library #25558

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

lib: adding support for modem to updatehub library #25558

wants to merge 1 commit into from

Conversation

mtahirbutt
Copy link
Contributor

This pull request addresses the support for slower air interfaces (modem for example) to updatehub. Originally updatehub was designed for high speed fixed LAN interfaces. Therefore, this implementation has been tested with DIGI XBC-M5-UT-001 XBee Cellular 3G modem with STM32L475 MCU BSP. It is expected that it should work with other interfaces as well. Some feedback is requested in this regard also.
Additionally there is also a bug fix for #24853.

@mtahirbutt mtahirbutt requested a review from nandojve as a code owner May 23, 2020 02:42
@mtahirbutt mtahirbutt marked this pull request as draft May 23, 2020 02:48
@zephyrbot
Copy link

zephyrbot commented May 23, 2020 •
edited
Loading

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@mtahirbutt mtahirbutt force-pushed the modem-support branch 3 times, most recently from 0abf325 to f7c0a04 Compare May 23, 2020 08:32
@mtahirbutt mtahirbutt closed this May 23, 2020
@mtahirbutt mtahirbutt reopened this May 23, 2020
@mtahirbutt mtahirbutt force-pushed the modem-support branch 2 times, most recently from 97b0784 to 1ba1853 Compare May 23, 2020 13:17
@nandojve
Copy link
Member

Hi @mtahirbutt, thank you for sharing this. Now is far more clear.

CC @otavio

@mtahirbutt mtahirbutt marked this pull request as ready for review May 23, 2020 16:05
@mtahirbutt
Copy link
Contributor Author

Hi @mtahirbutt, thank you for sharing this. Now is far more clear.

CC @otavio

@nandojve the PR is ready for review, plz also add second reviewer.

thanks.

jukkar
jukkar requested changes May 25, 2020
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Please check the code for style issues, there were lot of them. Also do not remove empty lines unnecessarily especially when they are not related to the actual functionality you are introducing.

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could we place these newly introduced variable here and below to ctx struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Having empty line here is good, please remove this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please introduce variables at the beginning of the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please place empty line after ending } (apply to here and other places in your commit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added spaces as you mentioned

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Indentation wrong

lib/updatehub/updatehub.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Having here empty lines is fine, why are you removing these

@mtahirbutt mtahirbutt force-pushed the modem-support branch 5 times, most recently from 138e7c8 to ce255e1 Compare May 27, 2020 04:59
mtahirbutt
mtahirbutt commented May 27, 2020
View reviewed changes
lib/updatehub/updatehub.c Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with new code, the previous places generates long line length warnings.

@jukkar
Copy link
Member

jukkar commented May 27, 2020

Thanks for the fixes, you can squash the fix commit to the first one and resubmit branch.

@mtahirbutt
Copy link
Contributor Author

Thanks for the fixes, you can squash the fix commit to the first one and resubmit branch.

thanks for the message. done as suggested.

@nandojve nandojve added the DNM This PR should not be merged (Do Not Merge) label May 27, 2020
@nandojve nandojve mentioned this pull request May 28, 2020
Merged
@hwilmers hwilmers mentioned this pull request May 28, 2020
Closed
@nandojve nandojve linked an issue May 28, 2020 that may be closed by this pull request
lib: updatehub: Corrupted updated when receiving CoAP duplicate packages #25704
Closed
@mtahirbutt
lib: adding modem support for the updatehub library
0eebd70 
Signed-off-by: Tahir Akram <[email protected]>

This PR addresses the support for slower air interfaces
(modem for example) to updatehub. Therefore, this
implementation has been tested with DIGI XBC-M5-UT-001
XBee Cellular 3G modem with STM32L475 MCU BSP.
@mtahirbutt mtahirbutt mentioned this pull request May 31, 2020
Closed
@nandojve nandojve mentioned this pull request Jun 10, 2020
Merged
@nandojve
Copy link
Member

We would like thank you @mtahirbutt by this contribution. It helps us to improve UpdateHub fixing 3 bugs, avoid unnecessary flash write, improve CoAP lib, and add support to slow wireless interfaces like modem.

This works was addressed on #25603, #25683 and #26093.

@nandojve nandojve closed this Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar requested changes

@nandojve nandojve Awaiting requested review from nandojve

Assignees
No one assigned
Labels
DNM This PR should not be merged (Do Not Merge)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lib: updatehub: Corrupted updated when receiving CoAP duplicate packages
4 participants
@mtahirbutt @zephyrbot @nandojve @jukkar