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

Mark position after execute for CRC retries #41

Merged
merged 2 commits into from Feb 14, 2017

Conversation

Projects
None yet
2 participants
@dwalkes
Contributor

dwalkes commented Feb 3, 2017

I've noticed when using in a noisy environment (or if I drop my phone in a Faraday Cage)
if I ever see a CRC error on a data packet all of the subsequent retries fail and the download fails.

I noticed if I print the CRC values expected they change on each retry
attempt. This seems wrong, since I believe the intent is just to re-send
the last chunk?

I suspect there's a mark() call missing in the success transfer case,
and the position of the last successful create->execute transfer should
be saved. The next retry transfer should reset to the mark position after the last
successful transfer.

If I add a call to mFirwmareStream.mark() in the CRC success case after
writeExecute() completes I am able to successfully recover from CRC errors with my setup.

Is this a correct change? Or am I missing something about how CRC retries are intended to work?

Mark position after execute for CRC retry
I've noticed when using in a noisy environment if I ever see a CRC error
all of the subsequent retries fail and the download fails.

I noticed if I print the CRC values expected they change on each retry
attempt.  This seems wrong, since I believe the intent is just to re-send
the last chunk.

I suspect there's a mark() call missing in the success transfer case,
and the position of the last successful create->execute transfer should
be saved. The next retry transfer should only reset to this mark position
and not the beginning of the file.

If I add a call to mFirwmareStream.mark in the CRC success case after
writeExecute() completes I am able to successfully recover from CRC errors.
@philips77

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 3, 2017

Thanks! This seems very correct. I'm on vacations now, but will merge it when get back to work on Wed.
One thing, could you add spaces after commas in lines you added?

philips77 commented on 18bdda9 Feb 3, 2017

Thanks! This seems very correct. I'm on vacations now, but will merge it when get back to work on Wed.
One thing, could you add spaces after commas in lines you added?

Clean up log messages, match code convention
Use single space after commas and between assignments.
@dwalkes

This comment has been minimized.

Show comment
Hide comment
@dwalkes

dwalkes Feb 5, 2017

Contributor

Thanks Aleksander, sorry I didn't match the coding convention with the previous commit. I think it's correct now. I've also cleaned up the logging as well. Let me know if you'd like any other changes.

Contributor

dwalkes commented Feb 5, 2017

Thanks Aleksander, sorry I didn't match the coding convention with the previous commit. I think it's correct now. I've also cleaned up the logging as well. Let me know if you'd like any other changes.

@philips77

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 5, 2017

Member

Space between . and " is redundant.

Space between . and " is redundant.

This comment has been minimized.

Show comment
Hide comment
@dwalkes

dwalkes Feb 5, 2017

Contributor

Sorry but I don't see which space you are referencing. I only see one . followed by "

Expected %08X but found %08X."

and there's no space between the . and " in this case. Can you please clarify?

Contributor

dwalkes replied Feb 5, 2017

Sorry but I don't see which space you are referencing. I only see one . followed by "

Expected %08X but found %08X."

and there's no space between the . and " in this case. Can you please clarify?

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 5, 2017

Member

Oh, sorry, I read it on my mobile and thought there is a space in this line and plus one in the concatenated string. Sorry, nvm.

Member

philips77 replied Feb 5, 2017

Oh, sorry, I read it on my mobile and thought there is a space in this line and plus one in the concatenated string. Sorry, nvm.

@philips77

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 5, 2017

Member

Ok, so one space and the code seems to be correct. As I said I'll test it when her back to work. I assume you've tested it well, so mine will be more like "testing".
Have a nice day!

Member

philips77 commented on 671ef3f Feb 5, 2017

Ok, so one space and the code seems to be correct. As I said I'll test it when her back to work. I assume you've tested it well, so mine will be more like "testing".
Have a nice day!

This comment has been minimized.

Show comment
Hide comment
@dwalkes

dwalkes Feb 5, 2017

Contributor

Hi Aleksander,

so one space and the code seems to be correct.

See comments above and let me know if I'm missing something.

I assume you've tested it well, so mine will be more like "testing".

I've tested by verifying the CRC errors I can induce on my Nexus 5 + Faraday Cage setup are retried successfully. I actually haven't been able to induce a case where all retries fail after this change, so the attempts >= MAX_ATTEMPTS branch is verified by code review only.

Full disclosure: I haven't actually tried this change with Nordic hardware, I'm using with a different project at the moment. See https://github.com/Trellis-Logic/sdu/

You might want to try the Faraday Cage test with Nordic hardware and prove retries work there, or if you have some other way to induce CRC errors in hardware you might want to use that.

Contributor

dwalkes replied Feb 5, 2017

Hi Aleksander,

so one space and the code seems to be correct.

See comments above and let me know if I'm missing something.

I assume you've tested it well, so mine will be more like "testing".

I've tested by verifying the CRC errors I can induce on my Nexus 5 + Faraday Cage setup are retried successfully. I actually haven't been able to induce a case where all retries fail after this change, so the attempts >= MAX_ATTEMPTS branch is verified by code review only.

Full disclosure: I haven't actually tried this change with Nordic hardware, I'm using with a different project at the moment. See https://github.com/Trellis-Logic/sdu/

You might want to try the Faraday Cage test with Nordic hardware and prove retries work there, or if you have some other way to induce CRC errors in hardware you might want to use that.

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 5, 2017

Member

I'm using two nRF Connect apps on two phones, one does DFU, other emulates DFU target (using macros).
But I trust and see that it should work.

Member

philips77 replied Feb 5, 2017

I'm using two nRF Connect apps on two phones, one does DFU, other emulates DFU target (using macros).
But I trust and see that it should work.

This comment has been minimized.

Show comment
Hide comment
@dwalkes

dwalkes Feb 5, 2017

Contributor

I'm using two nRF Connect apps on two phones, one does DFU, other emulates DFU target (using macros)

That's a great way to do the testing. You could probably simulate all kinds of interesting error conditions like CRC errors that way too. I assume the emulation project isn't open source? I don't see it in the Nordic Semiconductor repository.

Contributor

dwalkes replied Feb 5, 2017

I'm using two nRF Connect apps on two phones, one does DFU, other emulates DFU target (using macros)

That's a great way to do the testing. You could probably simulate all kinds of interesting error conditions like CRC errors that way too. I assume the emulation project isn't open source? I don't see it in the Nordic Semiconductor repository.

This comment has been minimized.

Show comment
Hide comment
@philips77

philips77 Feb 5, 2017

Member

Well, the crcs and responses depend on the firmware being sent. But I can share the zip and number of macros that need to pass if anyone wanted to run them on their own.

Member

philips77 replied Feb 5, 2017

Well, the crcs and responses depend on the firmware being sent. But I can share the zip and number of macros that need to pass if anyone wanted to run them on their own.

@dwalkes

This comment has been minimized.

Show comment
Hide comment
@dwalkes

dwalkes Feb 5, 2017

Contributor

The space here is because I'm appending to the string, not sure if that was obvious or if it might be what you were referring to above. The resulting output print in a retry case looks like this:

I/DfuImpl: Uploading firmware...
I/DfuImpl: Sending Calculate Checksum command (Op Code = 3)
I/DfuImpl: Checksum received (Offset = 12288, CRC = A5DD7E8B)
I/DfuImpl: CRC does not match! Expected AE5D19A2 but found A5DD7E8B. Retrying...(2/3)

The space here is because I'm appending to the string, not sure if that was obvious or if it might be what you were referring to above. The resulting output print in a retry case looks like this:

I/DfuImpl: Uploading firmware...
I/DfuImpl: Sending Calculate Checksum command (Op Code = 3)
I/DfuImpl: Checksum received (Offset = 12288, CRC = A5DD7E8B)
I/DfuImpl: CRC does not match! Expected AE5D19A2 but found A5DD7E8B. Retrying...(2/3)

@philips77 philips77 merged commit 032a133 into NordicSemiconductor:release Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment