Thread multicast DFU support #95

rlubos · 2017-10-26T12:28:51Z

This PR adds support for Thread multicast DFU, that will be added in future Thread SDK release.

mrodem · 2017-11-03T08:35:48Z

nordicsemi/__main__.py

@@ -763,16 +774,13 @@ def convert_version_string_to_int(s):
              type=click.STRING)
 @click.option('-a', '--address',
              help='Device IPv6 address. If address is not specified then perform DFU'
-                   + 'on all capable devices.',
+                   + 'on all capable devices. If multicast address is specified (FF03::1),'
+                   + 'perform multicast DFU.',


Some whitespace getting lost in the string concatenation here. This will output "... DFUon all capable ..." and "...
(F03::1),perform multicast ...". Also, the +'es can be removed.

mrodem · 2017-11-03T08:44:24Z

nordicsemi/__main__.py

+              type=click.FLOAT)
+@click.option('-d', '--diag',
+              help='Request and print diagnostic information.' +
+                   'Option value determines how many times the diagnostic request is sent. ' +


Should add space between "information." and "Option". The +'es can be removed.

mrodem · 2017-11-03T08:51:18Z

nordicsemi/__main__.py

+    opts.reset_suppress = reset_suppress
+    opts.mcast_dfu = mcast_dfu
+
+    print(opts.diag_addr)


This should probably be removed?

mrodem · 2017-11-03T09:19:28Z

nordicsemi/__main__.py

+              multiple = True)
+@click.option('-nv', '--non_verbose',
+              help='Disables logger output.',
+              is_flag=True)


What is the use case for non-verbose? There is already a --verbose flag for specifying log level, so it would be good if we could use that for thread DFU as well.

mrodem · 2017-11-03T09:52:39Z

nordicsemi/__main__.py

-        click.echo("Press <ENTER> terminate")
+        transport.open()
+        # Delay DFU trigger until NCP promotes to a router (5 seconds by default)
+        time.sleep(6.0)


Comment says 5 secs, but sleeping 6 secs.

mrodem · 2017-11-05T20:35:32Z

nordicsemi/thread/dfu_server.py

+                                                                       num,
+                                                                       _bmp_to_str(bmp)))
+
+        # TODO: fix me


Is there something that should be fixed here?

mrodem · 2017-11-05T20:36:49Z

nordicsemi/thread/dfu_server.py

+        elif (path == 'f'):
+            resource = self.image_resource
+
+        for i in range(63):


Why 63? Consider using a named constant to make it more clear.

mrodem · 2017-11-05T20:41:37Z

nordicsemi/thread/dfu_server.py

+            self._dump_diagnostic_report()
+        elif (self.opts.diag == -1):
+            time.sleep(20)
+            self._dump_diagnostic_report()


Seems that there is some special behavior here if --diag is -1. Should it be documented in the CLI help output?

mrodem · 2017-11-05T20:51:10Z

nordicsemi/thread/tncp.py

-        logger.debug("Sending datagram {} {} {} {}".format(self._src_addr,
+        if (dest.addr.is_multicast):            
+            rloc16 = self._wpan.prop_get_value(SPINEL.PROP_THREAD_RLOC16)
+            src_addr = ipaddress.ip_address(self._ml_prefix + '\x00\x00\x00\xff\xfe\x00' + struct.pack('>H', rloc16))


Could this be made more readable? Or add a comment to explain what is being done?

mrodem · 2017-11-05T20:56:45Z

nordicsemi/thread/dfu_thread.py

@@ -68,10 +69,13 @@ def _get_file_names(manifest):
 	logger.info("Image type {} found".format(data_attrs[0]))
 	return firmware.dat_file, firmware.bin_file

-def create_dfu_server(transport, zip_file_path, prefix):
+def create_dfu_server(transport, zip_file_path, opts):


Would be good if you could document what can be passed in as opts.

CLAassistant · 2017-11-07T11:08:57Z

All committers have signed the CLA.

rlubos · 2017-11-07T11:13:46Z

Thank you for the review. I've applied appropriate fixes and removed some obsoletecode , that we've considered as no longer needed - mostly regarding the diag option.

mrodem · 2017-11-08T09:53:20Z

Thanks 👍 I see that there is also a conflict in nordicsemi/__main__.py that needs to be resolved. Also, the CLA must be signed before the PR can be merged.

bihanssen · 2017-11-08T10:29:17Z

nordicsemi/version.py

@@ -37,4 +37,4 @@

 """ Version definition for nrfutil. """

-NRFUTIL_VERSION = "3.3.2"
+NRFUTIL_VERSION = "3.4.0"


@rlubos, you asked me if the version should be bumped as part of the PR, which I confirmed. However, in hindsight, I think it is better to leave out the version change here, and we'll instead deal with setting the correct version when we do the next release. Sorry for the mixup.

Ok, I've removed that commit.

rlubos · 2017-11-08T10:59:46Z

Ok, so I've signed the CLA (I wasn't aware that Noridc employees have to do this as well) and removed the commit that bumbed up the version.

Is there any change though that updated version will be released any time soon? We wanted to point out in our SDK's documentation to that specific version, and we're releasing soon.

IvanSanchez · 2017-11-10T15:57:12Z

This is failing on one of my Linux machines with the following error:

$ nrfutil dfu thread -pkg /tmp/Thread_DFU-test/app_dfu_package.zip --port /dev/ttyACM0  -f
pkg /tmp/Thread_DFU-test/app_dfu_package.zip --port /dev/ttyACM0  -f
Address not specified. Using ff03::1 (all Thread nodes)
Using connectivity board at serial port: /dev/ttyACM0
Board already flashed with connectivity firmware.
Opening serial to /dev/ttyACM0 @ 115200
2017-11-10 16:55:45,473 Failed to set property PROP_IPv6_ICMP_PING_OFFLOAD
Traceback (most recent call last):
  File "/home/ivan/devel/pc-nrfutil/nordicsemi/__main__.py", line 918, in thread
    transport.open()
  File "/usr/local/lib/python2.7/dist-packages/nordicsemi/thread/tncp.py", line 211, in open
    if (not self._attach_to_network()):
  File "/usr/local/lib/python2.7/dist-packages/nordicsemi/thread/tncp.py", line 102, in _attach_to_network
    self._set_property(*props)
  File "/usr/local/lib/python2.7/dist-packages/nordicsemi/thread/tncp.py", line 87, in _set_property
    raise Exception("Failed to set property {}".format(self.__class__._propid_to_str(propid)))
Exception: Failed to set property PROP_IPv6_ICMP_PING_OFFLOAD

@rlubos I'd rather make this work locally at least once before merging. Any clues?

wbober · 2017-11-13T08:50:33Z

@IvanSanchez
The issue you see occurs sometimes after board has been flashed with ncp connectivity firmware. Usually a single hard reset helps.

IvanSanchez · 2017-11-13T09:30:28Z

@wbober No luck. It's either Failed to set property PROP_IPv6_ICMP_PING_OFFLOAD after running with -f, or Failed to reset NCP. Please flash connectvity firmware. after a reset or power-cycle.

rlubos · 2017-11-13T10:11:13Z

@IvanSanchez I do not have solution, just some feedback.
I've tried nrfutil on Linux for the first time (Ubuntu 16.04 on VirtualBox) and experienced the same issue you described. But in my case, power cycle helped, and since then I've not experienced such a failure (iv'e tried like 10 times). I'm running nrfutil with -f option all the time.

IvanSanchez · 2017-11-13T15:08:13Z

Apparently this runs, but I haven't been able to complete a multicast DFU, and also I'm not totally happy with some of the error messages and behaviour.

I'll merge this, but I'll also create a few issues about those.

mrodem reviewed Nov 6, 2017

View reviewed changes

Add Thread multicast DFU support.

f612900

bihanssen reviewed Nov 8, 2017

View reviewed changes

Review fixes.

0267924

bihanssen approved these changes Nov 10, 2017

View reviewed changes

IvanSanchez self-requested a review November 10, 2017 16:00

Merge branch 'master' into thread_multicast_support

33bf12c

IvanSanchez merged commit 202fc28 into NordicSemiconductor:master Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread multicast DFU support #95

Thread multicast DFU support #95

rlubos commented Oct 26, 2017 •

edited

mrodem Nov 3, 2017

mrodem Nov 3, 2017

mrodem Nov 3, 2017

mrodem Nov 3, 2017

mrodem Nov 3, 2017

mrodem Nov 5, 2017

mrodem Nov 5, 2017

mrodem Nov 5, 2017

mrodem Nov 5, 2017

mrodem Nov 5, 2017

CLAassistant commented Nov 7, 2017 •

edited

rlubos commented Nov 7, 2017

mrodem commented Nov 8, 2017

bihanssen Nov 8, 2017

rlubos Nov 8, 2017

rlubos commented Nov 8, 2017

IvanSanchez commented Nov 10, 2017

wbober commented Nov 13, 2017

IvanSanchez commented Nov 13, 2017

rlubos commented Nov 13, 2017

IvanSanchez commented Nov 13, 2017

Thread multicast DFU support #95

Thread multicast DFU support #95

Conversation

rlubos commented Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 7, 2017 • edited

rlubos commented Nov 7, 2017

mrodem commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlubos commented Nov 8, 2017

IvanSanchez commented Nov 10, 2017

wbober commented Nov 13, 2017

IvanSanchez commented Nov 13, 2017

rlubos commented Nov 13, 2017

IvanSanchez commented Nov 13, 2017

rlubos commented Oct 26, 2017 •

edited

CLAassistant commented Nov 7, 2017 •

edited