Moved triplicated code and made some fixes #306
Conversation
The following, now replaced, piece of code puzzled me for a while and
deserves a mention (see comments added):
```
nordicsemi/dfu/dfu_transport_ble.pu
Class DfuAdapter:
def get_message(self):
# reset parser
current_state = Slip.SLIP_STATE_DECODING
finished = False
decoded_data = []
while finished == False:
byte = self._serial.read(1)
if byte:
# "first element of byte of bytearray() of len(1)"
(byte) = struct.unpack("B", byte)[0]
# modify by reference
(finished, current_state, decoded_data) = Slip.decode_add_byte(
byte, decoded_data, current_state
)
# else on read timeout - the only case where serial.read(1) returns
# empty bytearray.
else:
# change slip parser state ...
current_state = Slip.SLIP_STATE_CLEARING_INVALID_PACKET
# ... then ignore new parser state and return
return None
return decoded_data
```
'object' is reserved python and means somehing else
black default config seems to conform to style in other modules
`DfuTransportSerial.DEFAULT_TIMEOUT = 30.0` and init `timeout` argument used in `__main__.py` (cli) but the serial transport never used them. instead it had a `DEFAULT_SERIAL_PORT_TIMEOUT = 1.0` (not used from from __main__)
hopefully more "intuitive" names
There was a problem hiding this comment.
A lot of great clean up and refactoring here. Good idea to run auto formatting on the code, the code looks a lot more coherent now.
All refactorings look ok as far as I can see. The CI tests pass fine, though as they are far from exhaustive we will do some extra testing before next release.
Thanks for the PR!
|
@5frank do you consider the PR complete, ok to merge? |
|
@bihanssen Thanks and yes, I consider it complete. I haven't really studied the tests but seems like at least some of the transport layers are tested? I was sort of hoping that is the case because I have a limited setup right now (no windows machine for testing ant for example). |
|
There are a number of unit tests being run, but none of them include actual hardware. Without it one can only reach a certain test confidence level. We are looking into adding hardware tests into the CI system for nrfutil, but for now it's manual testing only. |
|
@bihanssen I see. Unless you have a setup and could do it I might have some time next week to test ble and serial transport (not ant) some more. |
|
Ok, good, then I'll wait a little with merging until some more ble and serial transport tests have been run. We have started some work on automation of hardware tests, but can't tell when it will be finalized. |
|
@5frank, some more automated tests running on physical hardware have been added as of PR #311. We made a quick run with your PR, and some of the bdd tests were failing. Could you try to pull in master and try to run the bdd tests locally? (https://github.com/NordicSemiconductor/pc-nrfutil/tree/master/tests/bdd) |
|
@bihanssen thanks! I will try run the tests and see what is failing. The devboard is unfortunately at the office and I'm working from home as so many others right now (- which is also an excuse of lack of time :) ) |
|
@bihanssen Perhaps this is a stupid question, but how do I run the tests on my local machine? I get a error on a missing |
|
@halkver thanks! Searching for "bdd tests" lead me to believe I needed |
|
Is this PR still something you want merged into master? This PR seems rather large, so tracking what these changes may do will be difficult. Of this is something you still feel like you want to add to the project feel free to re-open it, but for now, I will close this PR, just to clean up the list of PRs and due to its age. |
|
@jornbh Unfortunately I do not have time to work on this now so closing seems correct. Hopefully I get back with a smaller PR containing at least some bugfixes. |
Opening PR to trigger your exquisite test pipeline. Might be something I missed.
If any names or terminology seems alien from a NordicSemi perspective, please tell and I change it!
Changes
Moved triplicated code from
dfu_transport{ant,ble,serial}.pyto base class indfu_transport.py. Only differences between ant, ble, and serial implementationwas:
antimplementation had a extra check on remainderif (expected_crc != response['crc']) or (remainder == 0):ble makes severeal attempts to send data package. added parameter for
this that defaults to a single attempt.
Run black code formatter which seems to conform to the style of other modules in this repo.
Sorry for the large diff!
Added comments
Changed
OP_CODEandRES_CODEfromdicttoIntEnum. pylint friendly, easy argument checks and converts nicely to strings. Names more or less according to C enums as they arewell documented (also the only documentation I found). Also, DFU operation command methods such as
__execute()replaced with_operation_command(OP_CODE.OBJ_EXECUTE)which is hopefully easier to understand.Passing
listtoserial.Serial().write()seems to be a undocumented feature. changed to bytearray.Changed
DFUAdapter.LOCAL_ATT_MTU = att_mtu- not threated as a constantRemoved DfuTransport.DEFAULT_DO_PING as not used
Fixes
in dfu_transport_serial.py - Exception do not have a
strerrormember(OSError).
in dfu_transport_ant.py - Exception do not have a
messagemember.timeout for serial was always 1 s.
DfuTransportSerial.DEFAULT_TIMEOUT = 30.0and inittimeoutargument usedin
__main__.py(cli) but the serial transport never used them. instead ithad a
DEFAULT_SERIAL_PORT_TIMEOUT = 1.0Findings - possible issues not fixed
Both ant and serial ignores response timeout for
OP_CODE.OBJ_CREATE(aka__create_object). It was not obvious this is what happened - mightnot be intentional? added warning when this occur.
This doesn't look right in
dfu_transport_serial.py:how to tell if
__ensure_bootloader()orSerial()raised the error?