OSAL APIs to Firmware read, write & System reboot/TLV32 for Linux

[manojnacsl]

Implement OSAL APIs (Firmware/slothdr read, write, System reboot + TLV32) for Linux

OSAL APIs definitions for Linux:

1. Firmware read
2. Firmware write
3. Firmware slot-header read
4. Firmware slot-header write
5. System reboot

TLVs added:

1. RebootRequest TLV32

Baseline issues fixed:

1. CGMSNotification TLV44 protobuf interop issue w/ FND

OSAL API signature:

/****************************************************************************
 * @fn   osal_system_reboot
 *
 * @brief Reboots system
 *
 * input parameters
 *  @param[in] NMSaddr NMS IPv6 address
 *
 * output parameters
 * @return returns 1 on success and 0 on failure
 *****************************************************************************/
osal_basetype_t osal_system_reboot(struct in6_addr *NMSaddr);

/****************************************************************************
 * @fn   osal_read_firmware
 *
 * @brief read firmware image from storage(file/flash)
 *
 * input parameters
 *  @param[in] slotid indicating RUN/UPLOAD/BACKUP slot
 *  @param[in] data pointer to uint8_t data array
 *  @param[in] size of data in bytes
 *
 * output parameters
 * @return returns 0 on success and -1 on error
 *****************************************************************************/
osal_basetype_t osal_read_firmware(uint8_t slotid, uint8_t *data, 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] data pointer to uint8_t data array
 *  @param[in] size of data in bytes
 *
 * output parameters
 * @return returns 0 on success and -1 on error
 *****************************************************************************/
osal_basetype_t osal_write_firmware(uint8_t slotid, uint8_t *data, uint32_t size);

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

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

/****************************************************************************
 * @fn   osal_copy_firmware
 *
 * @brief Copy firmware image and slot header data from source to dest slot
 *
 * input parameters
 *  @param[in] source_slotid and dest_slotid indicating RUN/UPLOAD/BACKUP slot
 *  @param[in] pointer to _Csmp_Slothdr slot structure
 *
 * output parameters
 * @return returns 0 on success and -1 on error
 *****************************************************************************/
osal_basetype_t osal_copy_firmware(uint8_t source_slotid, uint8_t dest_slotid, Csmp_Slothdr *slot);

[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.

[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.

[silabs-ThibautC]

I only wanted to cover all the OSAL API updates or additions that could be required to perform a firmware update. We can talk about that in a second PR, I agree.

The generic function I'd expect to see in a generic API would:

• Write in a storage: that is covered by your proposal
• Read from a storage: that is covered by your proposal. That's said I am not sure it is really needed. If I understand correctly it is currently used to read an image from a file to access the file meta information in the future (here for instance?) . Am I correct?
• Select an image to boot from: an equivalent of write_fw_img
• Reboot

The definition of _Csmp_Slothdr is not generic enough and should probably be platform-dependent. In other words, an abstract definition of the storage location should be in osal.h. We cannot afford this kind of array:
uint8_t image[CSMP_FWMGMT_SLOTIMG_SIZE];

[manojnacsl]

Please review/approve to conclude this PR. Thanks.

[paduffy]

Good progress.

[paduffy]

Looks fine to me.

[silabs-ThibautC]

The API modifications will impact the work already done, but it looks good enough. We'll need to find another way of pushing new OSAL APIs in the future.