OSAL APIs to read/write firmware

[manojnacsl]

Proposed OSAL APIs to read/write firmware

/****************************************************************************
 * @fn   osal_read_firmware
 *
 * @brief read firmware image from storage(file/flash)
 *
 * input parameters
 *  @param[in] slotid indicating RUN/UPLOAD/BACKUP slot
 *  @param[in] pointer to _Csmp_Slothdr slot structure
 *  @param[in] size of _Csmp_Slothdr slot structure
 *
 * output parameters
 * @return returns 0 on success and -1 on error
 *****************************************************************************/
osal_basetype_t osal_read_firmware(uint8_t slotid, void* slot, uint32_t size);

/****************************************************************************
 * @fn   osal_write_firmware
 *
 * @brief write firmware image to storage(file/flash)
 *
 * input parameters
 *  @param[in] slotid indicating RUN/UPLOAD/BACKUP slot
 *  @param[in] pointer to _Csmp_Slothdr slot structure
 *  @param[in] size of _Csmp_Slothdr slot structure
 *
 * output parameters
 * @return returns 0 on success and -1 on error
 *****************************************************************************/
osal_basetype_t osal_write_firmware(uint8_t slotid, void* slot, uint32_t size);

[ismilak]

Hi @manojnacsl !
I tested all the platforms and there are build issues for all of them. The main target linux platform doesn't work neither.
I note that there was an other error introduced by previous PR (osal/HEAD~1)

My current errors are from the linux build:

$ ./build.sh linux
....

./osal/linux/osal_linux.c:432:10: error: use of undeclared identifier 'RUN_IMAGE'
    case RUN_IMAGE:
         ^
./osal/linux/osal_linux.c:435:10: error: use of undeclared identifier 'UPLOAD_IMAGE'
    case UPLOAD_IMAGE:
         ^
./osal/linux/osal_linux.c:438:10: error: use of undeclared identifier 'BACKUP_IMAGE'
    case BACKUP_IMAGE:
         ^
./osal/linux/osal_linux.c:466:10: error: use of undeclared identifier 'RUN_IMAGE'
    case RUN_IMAGE:
         ^
./osal/linux/osal_linux.c:469:10: error: use of undeclared identifier 'UPLOAD_IMAGE'
    case UPLOAD_IMAGE:
         ^
./osal/linux/osal_linux.c:476:46: error: member reference base type 'void' is not a structure or union
      bytes = fwrite((uint8_t*) &slot[slotid].image, sizeof(uint8_t), slot[slotid].filesize, file);
                                 ~~~~~~~~~~~~^~~~~~
./osal/linux/osal_linux.c:476:83: error: member reference base type 'void' is not a structure or union
      bytes = fwrite((uint8_t*) &slot[slotid].image, sizeof(uint8_t), slot[slotid].filesize, file);
                                                                      ~~~~~~~~~~~~^~~~~~~~~
./osal/linux/osal_linux.c:482:10: error: use of undeclared identifier 'BACKUP_IMAGE'
    case BACKUP_IMAGE:

[manojnacsl]

Hi @manojnacsl ! I tested all the platforms and there are build issues for all of them. The main target linux platform doesn't work neither. I note that there was an other error introduced by previous PR (osal/HEAD~1)

My current errors are from the linux build:

$ ./build.sh linux
....

./osal/linux/osal_linux.c:432:10: error: use of undeclared identifier 'RUN_IMAGE'
    case RUN_IMAGE:
         ^
./osal/linux/osal_linux.c:435:10: error: use of undeclared identifier 'UPLOAD_IMAGE'
    case UPLOAD_IMAGE:
         ^
./osal/linux/osal_linux.c:438:10: error: use of undeclared identifier 'BACKUP_IMAGE'
    case BACKUP_IMAGE:
         ^
./osal/linux/osal_linux.c:466:10: error: use of undeclared identifier 'RUN_IMAGE'
    case RUN_IMAGE:
         ^
./osal/linux/osal_linux.c:469:10: error: use of undeclared identifier 'UPLOAD_IMAGE'
    case UPLOAD_IMAGE:
         ^
./osal/linux/osal_linux.c:476:46: error: member reference base type 'void' is not a structure or union
      bytes = fwrite((uint8_t*) &slot[slotid].image, sizeof(uint8_t), slot[slotid].filesize, file);
                                 ~~~~~~~~~~~~^~~~~~
./osal/linux/osal_linux.c:476:83: error: member reference base type 'void' is not a structure or union
      bytes = fwrite((uint8_t*) &slot[slotid].image, sizeof(uint8_t), slot[slotid].filesize, file);
                                                                      ~~~~~~~~~~~~^~~~~~~~~
./osal/linux/osal_linux.c:482:10: error: use of undeclared identifier 'BACKUP_IMAGE'
    case BACKUP_IMAGE:

@ismilak
Please note this is a draft PR opened to primarily discuss on the proposal for Firmware read/write OSAL APIs. This is not the final change, but interim changes to conclude on the OSAL APIs.
Let's focus to finalize on the OSAL APIs (while we can fix the build errors as well in parallel)

Regarding the earlier build errors for FreeRTOS/EFR32, as informed in the previous call - our intent was to only add the platform agnostic CSMP encode/decode changes for FreeRTOS/EFR32 in the interest of time - there could be trivial build errors on these platforms which could be addressed during integration. Linux taget built fine and was used for manual testing.

[ismilak]

Should the enumeration be in osal.h? It will be required for other platforms too. If csmp_info is a library header, the enumeration should be moved into osal.h

[silabs-ThibautC]

I agree, the slots are owned by the osal an consumed by the csmp agent library

[manojnacsl]

@ismilak @silabs-ThibautC - thanks for your feedback. Sounds good, I'll move the declaration of xxx_IMAGE enum and _Csmp_Slothdr to osal.h and accordingly update the API signature.

[ismilak]

My recommendations for the new APIs:

• slotid type should be osal_basetype_t to be consistent
• _Csmp_Slothdr can be defined in osal_platform_types.h. In this case it can be portable for different platforms, using void* to access raw data is not a "safe" solution. Using appropriate type has more benefits.
• size should be osal_ssize_t or we can introduce a new unsigned size type (this argument is not required if the slot hdr is defined in osal_platform_types.h )

[manojnacsl]

@ismilak - thanks for your feedback. Sounds good, I'll move the declaration of xxx_IMAGE enum and _Csmp_Slothdr to osal.h and accordingly update the API signature.

[silabs-ThibautC]

I still think we are missing an OSAL API for rebooting.

That said.

After reviewing how the OTA DFU TLV handlers were implemented I understand why there are none. I was surprised to see that all of them were in a platform-specific file or folder. I didn't pay attention to that detail when I did my first quick review in December. I was expecting the OTA DFU logic (e.g., when imageBlock_post is called, do what you have to do then call osal_write_firmware, and so on) to be common to all platforms and located in a common folder.

As it stands, we theoretically wouldn't need the OSAL read/write functions. We could call SiliconLabs APIs in efr32_wisun_tlvs.c. However, there's a lot of code duplication. We have 1168 common lines (out of 1319) between efr32_wisun_tlvs.c and linux_tlvs.c. This increases the maintenance burden and will slow down the adoption of new platforms. It's critical to minimize platform-specific code.

As a CSMP agent consumer, what I'd like to see very explicitly exposed in the repo is what part of the repo is ready to be consumed as-is and which needs to be done to have a basic application. My vision is that it should be done in sample/
As a CSMP agent platform provider, what I'd like to see very explicitly exposed in the repo is what I need to develop to make my platform available to consumers. My vision is that it should be done in osal/.
Today, sample/ contains a bit of both. It gives the false impression the consumer could have a lot to do while he will not have to touch a single line of what's in efr32_wisun_tlvs.c once it is complete.

[manojnacsl]

I still think we are missing an OSAL API for rebooting.

That said.

After reviewing how the OTA DFU TLV handlers were implemented I understand why there are none. I was surprised to see that all of them were in a platform-specific file or folder. I didn't pay attention to that detail when I did my first quick review in December. I was expecting the OTA DFU logic (e.g., when imageBlock_post is called, do what you have to do then call osal_write_firmware, and so on) to be common to all platforms and located in a common folder.

As it stands, we theoretically wouldn't need the OSAL read/write functions. We could call SiliconLabs APIs in efr32_wisun_tlvs.c. However, there's a lot of code duplication. We have 1168 common lines (out of 1319) between efr32_wisun_tlvs.c and linux_tlvs.c. This increases the maintenance burden and will slow down the adoption of new platforms. It's critical to minimize platform-specific code.

As a CSMP agent consumer, what I'd like to see very explicitly exposed in the repo is what part of the repo is ready to be consumed as-is and which needs to be done to have a basic application. My vision is that it should be done in sample/ As a CSMP agent platform provider, what I'd like to see very explicitly exposed in the repo is what I need to develop to make my platform available to consumers. My vision is that it should be done in osal/. Today, sample/ contains a bit of both. It gives the false impression the consumer could have a lot to do while he will not have to touch a single line of what's in efr32_wisun_tlvs.c once it is complete.

A reboot OSAL API can be implemented and the plan to implement this API was discussed to be implemented along with Reboot_Request TLV32 as they are directly related.

The rest of the comments regarding xxx_tlvs.c implementation code for firmware upgrade is not very clear here and could be discussed in our sync-up call. Though there could be code which seems similar across osal platforms, the current implementation is inline to segregating code across Agent and platform specific Sample application - we need to discuss this further for better clarity on the proposal. Thanks.

[manojnacsl]

@silabs-ThibautC : As informed earlier in our discussions, the scope of this PR is to propose firmware read/write OSAL APIs. We had also confirmed earlier that OSAL API for reboot could be implemented along with TLV32 as a separate PR.

Further restructuring/optimizing of existing code(firmware upgrade, etc.,) being proposed now are out of scope of this PR and let's take it up for discussion separately as your FreeRTOS/EFR32 integration progresses. Thanks.