Device-os: Platform gcc broken in v1.2.1-rc.3 due to fclose(nullptr) at startup

[elcojacobs]

Bug Report

When building v1.2.1-rc.3 with PLATFORM=gcc and using gcc 8.3, the latest RC introduced some new functions that cause an exception at startup.
The code is excluded for unit tests, so it seems that your test environment is not running a gcc build outside of unit testing.

ota_flash_hall.cpp has:

hal_update_complete_t HAL_FLASH_End(hal_module_t* mod)
{
     fclose(output_file);
     output_file = NULL;
     return HAL_UPDATE_APPLIED;
}

called by:

hal_update_complete_t HAL_FLASH_ApplyPendingUpdate(hal_module_t* module, bool dryRun, void* reserved)
{
    HAL_FLASH_End(module);
    return HAL_UPDATE_APPLIED_PENDING_RESTART;
}

called by:

if (HAL_FLASH_ApplyPendingUpdate(nullptr /*module*/, false /*dryRun*/, nullptr /*reserved*/)==HAL_UPDATE_APPLIED_PENDING_RESTART) {
        // the regular OTA update delays 100 milliseconds so maintaining the same behavior.
        HAL_Delay_Milliseconds(100);
        HAL_Core_System_Reset_Ex(RESET_REASON_UPDATE, 0, nullptr);
    }

Result: fclose(nullptr) -> exception. :disappointed:

[elcojacobs]

Just avoiding closing a nullptr is not a fix. This avoids an exception, but the system still thinks it has updated and exits.

    if (output_file)
    {
        fclose(output_file);
    }

Why the function is called with a nullptr for module anyway, I don't know. It doesn't make sense.

[technobly]

Thanks for the bug report @elcojacobs ! I have added this to fix before 1.2.1 defaults. May I ask how you are using the GCC build?

[elcojacobs]

We use it for integration testing and development.
We develop a brewing temperature controller. The server that communicates with the photon/p1 runs on a bunch of docker containers. Each user runs it locally, most often on a Raspberry Pi.

When developing the backend microservices and front end, it is very useful to be able to simulate a controller instead of testing on actual hardware.

The simulator is built as part of our firmware CI and packaged as a docker container:
https://github.com/BrewBlox/brewblox-firmware/tree/develop/docker/

This enables us to start it up with our other docker containers when we are working on the front end:
https://github.com/BrewBlox/brewblox-ui/blob/develop/docker-compose.yml

Whenever we run npm start in the UI repo, it brings up all backend containers in a known state, configures the (simulated) controller with a preset collection of temperature control blocks and runs the UI in dev mode.

[elcojacobs]

Thanks for the bug report @elcojacobs ! I have added this to fix before 1.2.1 defaults. May I ask how you are using the GCC build?

Seems this was not included in 1.2.1 :(

[technobly]

Yes sorry about that. We did discuss it internally and had to make a call to get 1.2.1 defaulted for multiple internal and external customers. We will include a fix in the next release. Have you tested 1.3.0-rc.1 yet and would that work for you or would you require a bug fix backported to 1.2.x? I can't say for sure we can do that, but I'd like to understand how this affects you.

[elcojacobs]

I think it only affects the gcc cross compiled app we use for testing.

I can fix it in code without bumping the system version we use on the actual hardware. Probably a nullptr check before fclose is enough.
However, I use device-os as a git submodule. It would be easier to point it to a new tag on your remote instead of changing the submodule to my own fork for a fix.

I could upgrade to 1.3.0-rc.1 but I thought it might be better to use the master release in production.
Because the fix is in gcc/ota_flash_hal.cpp, it would not affect firmware builds, just the cross compile and the system binaries will be untouched.

[m-mcgowan]

If you want a quick patch, simply comment out the function body

hal_update_complete_t HAL_FLASH_ApplyPendingUpdate(hal_module_t* module, bool dryRun, void* reserved)
{
    return HAL_UPDATE_ERROR;
}

I'm trying to make a test for this but getting hamstrung on getting the GCC build to work on OS X.

[elcojacobs]

We build the (cross compiled) firmware in a docker container, perhaps that's an option. We also run the simulated firmware in a docker container.
Since docker is platform agnostic, that should work on OSX.

https://github.com/BrewBlox/brewblox-firmware/blob/develop/docker/compiler/Dockerfile

https://github.com/BrewBlox/brewblox-firmware/blob/develop/.vscode/tasks.json#L57