Skip to content
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

OTA update fails with "fatal" error code 0 #2785

Closed
donmsmall opened this issue Jan 31, 2025 · 3 comments · Fixed by #2793
Closed

OTA update fails with "fatal" error code 0 #2785

donmsmall opened this issue Jan 31, 2025 · 3 comments · Fixed by #2793
Labels
waiting for feedback Requires response from original poster

Comments

@donmsmall
Copy link

donmsmall commented Jan 31, 2025

When using the example for HTTPUpdate->HttpUpdate example I get the following error reported from the callbacks:

CALLBACK:  HTTP update process started
CALLBACK:  HTTP update fatal error code 0
HTTP_UPDATE_FAILD Error (0): 

I assumed that a "fatal" error should have a valid reason other than 0 - UPDATE_ERROR_OK.

Turned on debug, saw that data was arriving and written to the underlying LittleFS file of "firmware.bin". Eventually found a cryptic message of lfs_write reported a -28. -28 in the lfs.h file is no space remaining.

In the Updater.cpp file I find the following call to LittleFS write

bool UpdaterClass::_writeBuffer() {
    if (_command == U_FLASH) {
        if (_bufferLen != _fp.write(_buffer, _bufferLen)) {
            return false;
        }
    } else {

_fp.write is returning a value of 0, a value != to _bufferLen and takes the return false path . Returning false ripples up to a the failed download message but not reporting source of the error. Noticing similar checks for not sufficient flash space I suggest the following fix.

bool UpdaterClass::_writeBuffer() {
    if (_command == U_FLASH) {
        if (_bufferLen != _fp.write(_buffer, _bufferLen)) {
            _setError(UPDATE_ERROR_SPACE);
            return false;
        }
    } else {

and the expected error message:

CALLBACK:  HTTP update process at 49152 of 577508 bytes...
_fp.write returns 4096
CALLBACK:  HTTP update process at 53248 of 577508 bytes...
_fp.write returns 0
CALLBACK:  HTTP update fatal error code 4
HTTP_UPDATE_FAILD Error (4): Update error: ERROR[4]: Not Enough Space

To duplicate this problem one must send a sufficiently large update file that is bigger than the configured flash size. I set it to FS: 64kb.

In the process of verification of this fix, I configured the flash size for (no FS) and I get the same problem from a different location in the code also from Updater.cpp:

    if (command == U_FLASH) {
        LittleFS.begin();
        _fp = LittleFS.open("firmware.bin", "w+");
        if (!_fp) {
#ifdef DEBUG_UPDATER
            DEBUG_UPDATER.println(F("[begin] unable to create file"));
#endif
            return false;
        }
        updateStartAddress = 0;  // Not used
    } else if (command == U_FS) {
        if (&_FS_start + size > &_FS_end) {
            _setError(UPDATE_ERROR_SPACE);
            return false;
        }

which fails the file open test, optionally prints the DEBUG_UPDATER message and returns false. Same incorrect error code is reported. Adding the _setError(UPDATE_ERROR_SPACE); function call after the check for (!fp) fixes this error path too.

Unlike the ESP32 partition table, the pico does not currently have the ability to have separate spiffs and littlefs flash locations. Both U_FLASH and U_FS use the same flash memory locations. I think I saw in another discussion a mention that 2MB is barely sufficient as it is. Based on this assumption for the pico, the same test comparing FS_start + size > _FS_end test could be applied before attempting even the LittleFS.begin for the earliest possible error. For future proofing and the possibility of larger flash sizes and the addition of a partition table similar to ESP32, it might not be the best idea to add this particular test,

I've tested the following snippet with correct behaviour with all flash size variations.

     if (command == U_FLASH) {
+        if (&_FS_start + size > &_FS_end) {
+            _setError(UPDATE_ERROR_SPACE);
+            return false;
+        }

        LittleFS.begin();
        _fp = LittleFS.open("firmware.bin", "w+");
        if (!_fp) {
#ifdef DEBUG_UPDATER
            DEBUG_UPDATER.println(F("[begin] unable to create file"));
#endif
+            _setError(UPDATE_ERROR_SPACE);
            return false;
        }
        updateStartAddress = 0;  // Not used
    } else if (command == U_FS) {
        if (&_FS_start + size > &_FS_end) {
            _setError(UPDATE_ERROR_SPACE);
            return false;
        }
@earlephilhower
Copy link
Owner

Thanks for the very detailed analysis and proposed solution. Your analysis looks good. Error checking and return values are always the second-to-last things I get around to (the last being documentation, of course! 😆 ). Good additional size check at the start, too. The RPI team did add partitions w/ROM support with the RP2350, but I've not looked into it and don't have any immediate plans to do so. If they are added, Updater.cpp will probably be the least trouble to fix...the OTA app and all filesystem and EEPROM and BT TLV will be where the pain is.

Would you like to make a PR so you get the credit in the git blame logs?

@earlephilhower earlephilhower added the waiting for feedback Requires response from original poster label Jan 31, 2025
earlephilhower added a commit that referenced this issue Feb 4, 2025
earlephilhower added a commit that referenced this issue Feb 4, 2025
@donmsmall
Copy link
Author

donmsmall commented Feb 4, 2025 via email

@earlephilhower
Copy link
Owner

No worries and thanks again for pointing out the problem and a solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Requires response from original poster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants