Add LwM2M firmware push/pull function #1049

rbtchc · 2017-08-09T08:31:45Z

Implement firmware push and pull functions

Add a state machine for handling the firmware update process (both push & pull)
Use http_parser to parse URI passed from the server

jukkar · 2017-08-09T12:47:06Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Please put space before and after the '+'

jukkar · 2017-08-09T12:48:29Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Please put empty line before the comment, looks a bit better that way.

jukkar · 2017-08-09T12:49:06Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

spaces missing before and after +

jukkar · 2017-08-09T12:49:18Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

same issue here with '+', can you do a search and fix all of these in one go

jukkar · 2017-08-09T12:52:44Z

subsys/net/lib/lwm2m/lwm2m_engine.c

empty line after }

jukkar · 2017-08-09T12:55:10Z

subsys/net/lib/lwm2m/lwm2m_engine.c

empty line after }

jukkar · 2017-08-09T12:56:01Z

subsys/net/lib/lwm2m/lwm2m_engine.c

add spaces before and after '*'

jukkar · 2017-08-09T12:56:28Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

empty line after }

which is fine here

jukkar · 2017-08-09T12:57:12Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

empty line after }

jukkar · 2017-08-09T12:59:19Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

I looks like you have extra empty line between variable declarations above.

rsalveti · 2017-08-09T19:23:44Z

subsys/net/lib/lwm2m/Kconfig

In reality this is just using the url parser, so wonder if we should split that out of the http later.

Size is also increased quite a bit by just building http_parser, which could probably be optimised if we could just split the url parse part of it.

I will add a patch to separate the URL parsing from header parsing.

rsalveti · 2017-08-09T19:32:32Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Easier to read if using "if (!IS_ENABLED(CONFIG_NET_IPV6)" instead.

rsalveti · 2017-08-09T19:32:42Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

rsalveti · 2017-08-09T19:36:38Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Shouldn't you also call NOTIFY_OBSERVER(5, 0, 3)?

rsalveti · 2017-08-09T19:37:54Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Same, also missing NOTIFY_OBSERVER.

rsalveti · 2017-08-09T19:45:15Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Spec: "Writing an empty string to Package Resource or to Package URI Resource, resets the Firmware Update State Machine: the State Resource value is set to Idle and the Update Result Resource value is set to 0."

So we should reset state machine even if state is != DOWNLOADED, by checking for data_len outside the if/else block.

From LwM2M specification figure 29, it looks like it only reset to state idle when it's currently at state downloaded and an empty package URI/resource is written.
That's why I put the state == STATE_DOWNLOADED check here.

If we reset the state machine at any state, then we have to do (1) cancel the firmware download and (2) cancel the firmware update.

I'm not sure which is the correct behavior. But from my point of view, it's simpler if we only accept the state machine reset at DOWNLOADED state.

Figure 29 seems to be a possible implementation indeed, but that doesn't cover the cancel update case, which is why my interpretation was more to reset at any point in time, at any state.

Looks like something to ask oma for clarification.

rsalveti · 2017-08-09T19:47:23Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Would also be nice to add similar checks to package_uri_write_cb.

I'm afraid that I did not catch your point.
What kind of checks do I have to add to package_uri_write_cb?

At least to reset the state machine if there is an empty write.

rsalveti · 2017-08-09T19:48:31Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

More natural to read if "state == STATE_DOWNLOADED".

rsalveti · 2017-08-09T19:52:10Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

We should probably define a set of possible return codes here, as an error is not necessarily integrity failed (another possible value is just simply firmware update failed, but it could also return unsupported package type).

Good suggestion.
I will map -EINVAL to RESULT_INTEGRITY_FAILED and others to RESULT_UPDATE_FAILED.
From LwM2M specification figure 29, it seems there are only three acceptable results which are RESULT_SUCCESS, RESULT_INTEGRITY_FAILED and RESULT_UPDATE_FAILED.

rsalveti · 2017-08-09T19:57:22Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Shouldn't this be RESULT_CONNECTION_LOST instead?

tbursztyka · 2017-08-10T06:43:12Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

add an empty line before the for ()

tbursztyka · 2017-08-10T06:43:53Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

break a line after a } always (unless it's another } etc..)

tbursztyka · 2017-08-10T06:44:09Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

remove that empty line

tbursztyka · 2017-08-10T06:45:31Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

indent seems wrong here, & should be below f

tbursztyka · 2017-08-10T06:46:22Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

which is fine here

tbursztyka · 2017-08-10T06:58:47Z

subsys/net/lib/lwm2m/lwm2m_engine.c

break a line before for () (see my previous comment about if/for/while etc..)

tbursztyka · 2017-08-10T07:01:17Z

subsys/net/lib/lwm2m/lwm2m_engine.c

Since you removed the useless else {}, you could do:

if (r) {
goto error;
}

and thus all the code block below could be indented back. Nicer to read, less imbrication of if/else etc...

tbursztyka · 2017-08-10T07:01:47Z

subsys/net/lib/lwm2m/lwm2m_engine.c

here would be you error label

tbursztyka · 2017-08-10T07:03:27Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

usually, in net stack, we try to have declaration with assignment as first lines, then the rest. So I would reverse the order here.

Delete the debug message directly

tbursztyka · 2017-08-10T07:05:03Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

declare inbuf and insize before len

mike-scott · 2017-08-10T08:48:08Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

I feel like we should be grouping all of the variables of the same types on a line. Same with the u16_t below.

It's actually ok to do both. No need to enforce grouping

mike-scott · 2017-08-10T08:48:52Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Same with grouping up variables of the same type here.

mike-scott · 2017-08-10T08:54:11Z

include/net/lwm2m.h

These 2 functions are only available if CONFIG_LWM2M_FIRMWARE_UPDATE_OBJ_SUPPORT is enabled.

mike-scott · 2017-08-10T09:03:08Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Are we sure that we want to assert here (even if CONFIG_ASSERT is enabled)? That seems a bit harsh for passing in an invalid state. Maybe ignore and print an error message? Especially since this is exposed in application code.

mike-scott · 2017-08-10T09:07:17Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Once again, I'm not sure an assert is the right behavior here. Consider a strongly worded error message?

I will replace assert w/ error messages.

mike-scott · 2017-08-10T09:23:08Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

I feel like lwm2m_firmware_set_update_state() and lwm2m_firmware_set_update_result() are performing data_validation and then a followup call to set related data. In discussions with @rsalveti we had plans to add a new data_validation_callback for data elements which would be called prior to assigning a value to the data element.

This would clean up several issues that I'm seeing here:

You're creating specific data element based functions to control how a data element is set. This doesn't scale well over time.

These functions in turn trigger related data element values to be set.

Instead, if we had a data_validation callback which could be assigned to a data element. The validation could be performed there (returning non-zero value when an invalid value is found). And if a valid value was found the follow up assignments of update_state could be handled in a post_write callback of update_result, etc.

Yup, it looks like a good idea if we could have a data validation callback when application want to update the value of the object resources. (I'm assuming that you are talking about adding callback to lwm2m_engine_set_xxx() functions.)
Do I create a ticker on JIRA for tracking it or ?

rsalveti · 2017-08-10T15:43:17Z

@rbtchc while we get all the firmware update review done, would you mind breaking up the fixes you have in a separated pull request? At least "net: lwm2m: fix registration content format and use plain/text", "net: lwm2m: save accept format in observe_node_data" and "net: lwm2m: fix erroneous TLV write", as they are good fixes that could be merged soon.

rbtchc · 2017-08-11T02:50:28Z

Here are the summary of the changes I made.

Create separate PRs (net: lwm2m: fix erroneous TLV write #1089, net: lwm2m: fix registration content format and use plain/text #1090, net: lwm2m: save accept format in observe_node_data #1091) for patches independent to firmware push/pull
Correct the coding style
Add b84b6f45a3a92c0aa6ef34b039792bff9ebe5356 for a smaller URL only parser
Add NOTIFY_OBSERVER() call when update_result/update_state is updated. Replace assert w/ error message

TODO

Clarify whether firmware update can be cancelled at any state or DOWNLOADED state only
File a ticket JIRA for implement data_validation callback(?)

mike-scott · 2017-08-11T04:38:00Z

JIRA for data validation callback is here: https://jira.zephyrproject.org/browse/ZEP-2500

rbtchc · 2017-08-11T08:06:35Z

Raised firmware update cancellation question on OpenMobileAlliance/OMA_LwM2M_for_Developers#219

Do we need to wait for the clarification before we merge the patches?
Or perhaps I could file a JIRA ticket to track and patch the behavior when clarification is made.

rbtchc · 2017-08-18T08:37:40Z

@jukkar @tbursztyka @mike-scott @rsalveti
Please take a look at the updated patches and let me know if there are any unaddress issues.
Thanks.

rbtchc · 2017-08-20T05:17:05Z

Rebase and resolve code conflicts according to the change dcb80f7
Diff: https://hastebin.com/egiliqigiv.hs

pfalcon · 2017-08-20T07:00:59Z

Rebase and resolve code conflicts according to the change dcb80f7

Yep, sorry guys, I meant to ping you about it, seeing that lwm2m code uses, but that slipped. @mike-scott , @rsalveti, @mbolivar FYI

mike-scott · 2017-08-20T19:18:33Z

@rbtchc Testing this PR again today / tomorrow and I'll respond back. But @jukkar may want to discuss the Kconfig option to enable HTTP parser only.

jukkar

I am ok with the HTTP URL parser only option, I was actually needing something like this myself.

jukkar · 2017-08-20T21:02:43Z

include/net/http_parser.h

Could add the config option in comment here as there are almost 200 lines inside this define block.
So something like this:
#endif /* !CONFIG_HTTP_PARSER_URL_ONLY */

jukkar · 2017-08-20T21:04:46Z

subsys/net/lib/http/Kconfig

Perhaps just say here "This option only enables URL parser of the http_parser library."

jukkar · 2017-08-20T21:05:29Z

subsys/net/lib/http/http_parser.c

Add the /* !CONFIG_HTTP_PARSER_URL_ONLY */ here

jukkar · 2017-08-20T21:05:40Z

subsys/net/lib/http/http_parser.c

jukkar · 2017-08-20T21:05:49Z

subsys/net/lib/http/http_parser.c

jukkar · 2017-08-20T21:06:01Z

subsys/net/lib/http/http_parser.c

jukkar · 2017-08-20T21:06:12Z

subsys/net/lib/http/http_parser.c

jukkar · 2017-08-20T21:09:25Z

subsys/net/lib/lwm2m/Kconfig

You need to add "select HTTP_PARSER" here too as when doing "select" in Kconfig, it will not recursively select other options.

What about my comment here, is the HTTP_PARSER option selected by some other way? What I mean by this is if you select HTTP_PARSER_URL_ONLY here, it will not select HTTP_PARSER automatically (at least that is how Kconfig worked before) i.e., recursive "select" is not implemented. But it might be that the HTTP_PARSER is selected automatically by some other mean in which case my concern is moot.

I did add "select HTTP_PARSER" at subsys/net/lib/http/Kconfig#L68 when config HTTP_PARSER_URL_ONLY is selected.
In this way, when LWM2M_FIRMWARE_UPDATE_PULL_SUPPORT is selected, both HTTP_PARSER & HTTP_PARSER_URL_ONLY will be selected as well.

Ok, I missed that.

jukkar · 2017-08-20T21:13:50Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

Isn't there now a compile error if you disable IPv6 as the any_addr6 is only found if IPv6 is enabled?

jukkar · 2017-08-20T21:14:58Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

And same thing for IPv4

rbtchc · 2017-08-21T01:54:20Z

Update according to @jukkar 's comment. Here's the diff: https://hastebin.com/vovevumeca.cs

rbtchc · 2017-08-21T05:41:33Z

Update to take care of error handling when failing to add block1 option to the out packet
Diff: https://hastebin.com/upupiguqub.cs

jukkar · 2017-08-21T07:29:17Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware_pull.c

BTW, I just submitted #1198 which provides net_ipaddr_parse() function that converts a IPv{4|6} address string to struct sockaddr. That function could probably be used here in order to save some memory, this is just something that could be considered here.

Good to know. I will apply those changes to the PR when they are merged.

mike-scott · 2017-08-29T21:07:06Z

@rbtchc I hate to ask, but do you think this PR could be rebased on top of #1255 ?

rbtchc · 2017-08-29T21:37:15Z

@mike-scott sure

rsalveti · 2017-08-30T01:31:40Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

I wonder if we should always check for data_len == 0 even when state is not downloading. That is just to protect the code flow as the state can be updated via other parts of the code (even when it shouldn't).

Just to confirm. Are you suggesting that we always check if "data_len == 0" and reset the state back to default?

Yes, my concern is what would happen when data_len == 0 but state is not what we expect to be (simply because state can be changed independently).

rsalveti · 2017-08-30T01:31:54Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Same here (data_len == 0).

rsalveti · 2017-08-30T01:34:03Z

subsys/net/lib/lwm2m/lwm2m_obj_firmware.c

Mind also adding a error message here in case this gets called when state == DOWNLOADING.

Will print out an error message here.

1. Parse firmware pull URI 2. Add lwm2m_firmware_get/set_update_cb() for applicaiton to register callback. This is because we want to check the update_state before we pass to the application 3. Add lwm2m_firmware_get/set_update_result() and lwm2m_firmware_get/set_update_stat() to manage the state trasnition as well as the sanity check Signed-off-by: Robert Chou <[email protected]>

OPAQUE resource type might/might not have data_ptr/data_len setup depending on the implementation. This introduces an issue that when an OPAQUE resource is written from the server side, the one w/ none setup will not be able to pass data to post_write_cb() Modify to setup data_ptr/data_len as incoming buffer and buffer size Signed-off-by: Robert Chou <[email protected]>

1. Add handling block1 option in handle_request(). The basic idea is to declare structure block_context at compiled time and use "token" as a key to pick up the on-going block cotext. It should be able to support multiple blockwise transfer concurrently 2. Use write callback implemented in lwm2m_obj_firmware to deal w/ the update state transition and than call the callback registered by the application 3. move default_block_size to lwm2m_engine.c to share btw lwm2m_engine and lwm2m_obj_firmware_pull Signed-off-by: Robert Chou <[email protected]>

rbtchc · 2017-08-31T08:40:39Z

Update to address @rsalveti 's concerns. Diff: https://hastebin.com/exixuzaxex.cs

Reset update state / result to default when empty string is received by either /5/0/0 or /5/0/1
Remove TODO comment
Response -EPERM (mapped to 4.05) method not allowed when executing 5/0/2 (update) but state is not downloaded
Fix unable to compile when option CONFIG_LWM2M_FIRMWARE_UPDATE_PULL_SUPPORT=n

mike-scott · 2017-09-01T18:10:50Z

Thanks for the update @rbtchc I'll try and fold these changes into #1255 (using this as a holding PR for 1.10 changes for now)

mike-scott · 2017-10-07T04:14:52Z

Probably safe to close this PR. I have rebased versions of these patches included in this PR: #4208

rbtchc · 2017-10-07T04:56:09Z

Close as @mike-scott mentioned

…rproject-rtos#1049) It was checking the length of the filename against the maximum JS file length. Native modules can be longer and were failing because of this. Signed-off-by: Brian J Jones <[email protected]>

This fixes Bluetooth logs that were not sent over RTT. Minor cleanup has been made to limit the number of ifdefs. > ACL Data RX: Handle 0 flags 0x02 dlen 11 zephyrproject-rtos#1049 83.117000 ATT: Handle Value Indication (0x1d) len 6 Handle: 0x0003 Data: 0100ffff = bt: bt_att: Unhandled ATT code 0x1d 83.117100 > HCI Event: Disconnect Complete (0x05) plen 4 zephyrproject-rtos#1050 84.247700 Status: Success (0x00) Handle: 0 Reason: Remote User Terminated Connection (0x13) Signed-off-by: Mariusz Skamra <[email protected]>

This fixes Bluetooth logs that were not sent over RTT. Minor cleanup has been made to limit the number of ifdefs. > ACL Data RX: Handle 0 flags 0x02 dlen 11 #1049 83.117000 ATT: Handle Value Indication (0x1d) len 6 Handle: 0x0003 Data: 0100ffff = bt: bt_att: Unhandled ATT code 0x1d 83.117100 > HCI Event: Disconnect Complete (0x05) plen 4 #1050 84.247700 Status: Success (0x00) Handle: 0 Reason: Remote User Terminated Connection (0x13) Signed-off-by: Mariusz Skamra <[email protected]>

rbtchc requested review from jukkar and tbursztyka as code owners August 9, 2017 08:31

jukkar requested a review from mike-scott August 9, 2017 08:44

jukkar added the net label Aug 9, 2017

jukkar requested changes Aug 9, 2017

View reviewed changes

rbtchc force-pushed the firmware_update branch from b8e183a to e558b2f Compare August 9, 2017 14:31

rsalveti suggested changes Aug 9, 2017

View reviewed changes

tbursztyka requested changes Aug 10, 2017

View reviewed changes

mike-scott reviewed Aug 10, 2017

View reviewed changes

rbtchc force-pushed the firmware_update branch 3 times, most recently from 818d04c to 4e35cf1 Compare August 10, 2017 14:54

rbtchc force-pushed the firmware_update branch from 4e35cf1 to 2bcbd71 Compare August 11, 2017 02:25

rbtchc force-pushed the firmware_update branch from 2bcbd71 to 4c3c810 Compare August 20, 2017 05:13

jukkar reviewed Aug 20, 2017

View reviewed changes

rbtchc force-pushed the firmware_update branch from 4c3c810 to 70fc343 Compare August 21, 2017 01:52

rbtchc force-pushed the firmware_update branch from 70fc343 to c0d0481 Compare August 21, 2017 05:39

rbtchc force-pushed the firmware_update branch from c0d0481 to ee49c3d Compare August 21, 2017 05:49

jukkar reviewed Aug 21, 2017

View reviewed changes

rbtchc force-pushed the firmware_update branch from ee49c3d to a60783b Compare August 21, 2017 14:06

mike-scott mentioned this pull request Aug 24, 2017

DO NOT MERGE: LwM2M holding PR for 1.10 #1255

Closed

jukkar previously approved these changes Aug 24, 2017

View reviewed changes

rbtchc mentioned this pull request Aug 28, 2017

LWM2M: fix handle_reqest() errors #1273

Merged

rsalveti reviewed Aug 30, 2017

View reviewed changes

rbtchc added 3 commits August 31, 2017 16:16

rbtchc dismissed jukkar’s stale review via db98d60 August 31, 2017 08:33

rbtchc force-pushed the firmware_update branch from 7e456cd to db98d60 Compare August 31, 2017 08:33

nashif added area: Networking and removed net labels Sep 23, 2017

rbtchc closed this Oct 7, 2017

MariuszSkamra mentioned this pull request Jun 7, 2022

Bluetooth: monitor: Fix sending logs over RTT #46336

Merged

Add LwM2M firmware push/pull function #1049

Add LwM2M firmware push/pull function #1049

Uh oh!

Conversation

rbtchc commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rbtchc commented Aug 9, 2017 •

edited

Loading

mike-scott Aug 10, 2017 •

edited

Loading