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

net: lwm2m: Publicize firmware block context #16369

Closed
wants to merge 1 commit into from
Closed

net: lwm2m: Publicize firmware block context #16369

wants to merge 1 commit into from

Conversation

yaomon
Copy link
Contributor

@yaomon yaomon commented May 24, 2019

No description provided.

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

You need a message and your signed-off-by in addition to the commit title

In such message could you explain the use case for this function?

@zephyrbot
Copy link
Collaborator

zephyrbot commented May 24, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@mike-scott
Copy link
Contributor

I apologize for the delay. My work schedule has been very busy and I was away on vacation for the last few weeks. I'm reviewing this now.

@mike-scott
Copy link
Contributor

I recommended squashing the 2nd patch back into the first, but overall, I wonder if this isn't a situation that the LwM2M subsys should just handle. Why leave it up to the client at all?

@jukkar
Copy link
Member

jukkar commented Jul 23, 2019

@mike-scott What should we do with this, merge or not to merge?

@mike-scott
Copy link
Contributor

@jukkar don't merge. There is still the open question concerning the 2nd patch.

@mike-scott
Copy link
Contributor

@yaomon can you move the brace change back into the first patch (so there is only 1 patch)?

@jukkar jukkar added the DNM This PR should not be merged (Do Not Merge) label Jul 23, 2019
@sunkyl
Copy link

sunkyl commented Jul 23, 2019

Yea, I'll try to get on it ASAP.

This change is to allow access to the firmware block context in order to
give the firmware update callback implementation an indication of when
to reset the flash context. Additionally, it allows for a validity check
between the total expected size downloaded and the actual size
downloaded. A simple implementation can check if the block context's
current downloaded blocks is equal to the expected block size in order
to determine if the flash context needs to be reset. This approach
seemed the simplest, and knowing the firmware block context can have
other purposes. This has been tested by accessing the block context
during the update in the block received callback and confirming that the
callback had information regarding the current downloaded bytes.

Fixes #16122

Signed-off-by: Kyle Sun <yaomon18@yahoo.com>
@yaomon
Copy link
Contributor Author

yaomon commented Jul 28, 2019

Ok, the commits have been squashed. Let me know what else needs to be changed. Thanks for taking the time to respond :)

@zephyrbot zephyrbot added the area: API Changes to public APIs label Jul 28, 2019
@pfalcon pfalcon removed their request for review August 8, 2019 15:11
@mike-scott mike-scott added this to the v2.1.0 milestone Aug 28, 2019
@mike-scott mike-scott added the Enhancement Changes/Updates/Additions to existing features label Aug 28, 2019
Copy link
Contributor

@mike-scott mike-scott left a comment

Choose a reason for hiding this comment

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

LGTM

@mike-scott
Copy link
Contributor

@yaomon I tagged this for v2.1 as the merge window has been closed for some time on v2.0. Thank you for submitting and fixing the change.

@dleach02 dleach02 modified the milestones: v2.1.0, v2.2.0 Dec 3, 2019
@nashif
Copy link
Member

nashif commented Feb 6, 2020

stale, closing. Reopen when there are updates...

@nashif nashif closed this Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: LWM2M area: Networking DNM This PR should not be merged (Do Not Merge) Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants