Nimble ota check crc of the receive sector before writing into the flash

[StrugglingBunny]

BLE nimble ota ,add check crc of the receive sector before writing to the flash.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "Check crc of the receive sector before write to the flash": summary looks empty type/action looks empty the commit message "NimBle ota add sector crc check for both encrypt image ota and non encrypt ota.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Nimble ota(Allow app to send the sector again if crc mismatch)": summary looks empty type/action looks empty the commit message "Nimble ota(Request the sector again if crc mismtach)": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

	Messages
📖	You might consider squashing your 4 commits (simplifying branch history).

👋 Hello StrugglingBunny, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 15d0cb1

[leeebo]

Hi @StrugglingBunny thanks for your contribution! We will review ASAP!

[strange969]

Hi @StrugglingBunny,

In existing implementation, we receive the entire data, write to flash, apply the SHA-256 algorithm once to check the integrity of the full data set. If the data is not intact, we do not mark the flash as active, otherwise the process continues and reboot happens. . This approach ensures that we only need to perform the integrity check once, after the full data is written to flash.

In the proposed patch, however, the integrity of each packet is being checked before it’s written to flash, meaning multiple checks occur throughout the process. This will increase the overall time for OTA ( which is not recommended).

Can you please help share more information as to understand what the goal is with this change ? Could you clarify the benefit of checking each packet before writing, instead of waiting until all data is received and checking once? Also, i observe in your solution, if any packet's CRC check fails, you are simply dropping the packet, and continue to receive others. If controller has sent the packet to host, it will not resend the packet , so dropping packet at host would effectively lose the data, so what is the expected behaviour in this case ?

Thank you !!

[StrugglingBunny]

Hi @strange969
Thanks for your review.There are some reasons and benefits if we check each packet before writing.
1: In existing implementation, we receive the entire data, write to flash, apply the SHA-256 algorithm once to check the integrity of the full data set. Then if the SHA-256 mismatch,user needs to do the whole process again,which is not acceptable in some scenario and also for user it is not a good experience.For example,bin file is almost 1.3MB,use the BLE to do the OTA will take around 2 min and also add a crc check will not increase overall ota time a lot .

2: For app ,the app of the product should know the status of the update process. Crc check result should be send from device to center（The README file of the ble ota example also mention that :"Server will check the total length and CRC of the data from the client, reply the correct ACK, and then start receive the next sector of firmware data.".）.

3:In the lastest commit,i increase the cur_sector only if the current sector crc match,if the current sector crc mismatch,device will response CRC ERR to notify the app,so the app can ether send the sector again（For the demo app,do not send crc error to the phone and do not increase the sector index in the device can let the app send the sector again ).so in this case ,the app size not need to restart the whole ota process again. or stop the ota process(If send the crc error,the app will stop the ota process and report ).

I found that ,lots of the OTA protocol have the crc check of each chunk and also have the re-send chunks function when needed, in fact this will not increase lots of the overall time of the OTA process and can sometime reduce the OTA overall time when there is some issues in some chunks.So I think it's necessary.

Thank you !!!

[strange969]

Hi @StrugglingBunny, thank you so much for your time and effort in reviewing and providing the patch. I truly appreciate your help!

I’ve applied the patch which you provided and tested it on my side. However, I noticed that when a CRC checksum mismatch occurs, the EspBleOTA app (Espressif app) doesn’t retransmit the packet and disconnects. If it's not too much trouble, could you kindly clarify which app you are using to check this behavior?

I am attaching the screenshot of logs below.

[StrugglingBunny]

Hi @strange969
Sorry for the late respond,the current demo app will stop the whole process while there is an error coming during the OTA.I'll spend some time to build the app and add this feature for you to test when i'm free .thanks.

[StrugglingBunny]

Hi @strange969
I just made some small changes based on the original demo app ,you can download this (https://github.com/StrugglingBunny/esp-ble-ota-android/releases/tag/esp_ble_ota) apk file
and test the function.Make sure to pull the latest change first(I also make some small changes on the embedded side).

Test log.
Resend function.

Resend 3 times still fail,center stop the OTA process and disconnect.

Thanks.

[strange969]

Hi @StrugglingBunny , I have tested the changes, and they are working perfectly for both normal OTA and Delta OTA. However, I encountered issue when testing with encrypted OTA and Protocomm OTA.

Could you please help clarify why this PR was closed? Is there something incorrect in the implementation, or are there additional changes required? Kindly let me know if I can assist in addressing any concerns.