CMUAbstract / tab

TAB reference implementations, examples, and documentation
Other
1 stars 1 forks source link

Add support for Bootloader Erase #11

Closed mablem8 closed 1 year ago

mablem8 commented 1 year ago
ChadTaylor17 commented 1 year ago

Commit: f2a79ae215806de59ef11b89aa0cfed860a55c3d

Tested Successfully: Bootloader mode: txcmd: bootloader_erase hw_id:0x1234 msg_id:0x0008 src:gnd dst:cdh reply: bootloader_ack hw_id:0x1234 msg_id:0x0008 src:cdh dst:gnd reason:0x01(erased)

App Mode: txcmd: bootloader_erase hw_id:0x1234 msg_id:0x0008 src:gnd dst:cdh reply: common_nack hw_id:0x1234 msg_id:0x0008 src:cdh dst:gnd

mablem8 commented 1 year ago

For future posts, link to the changes that address each issue bullet:

ChadTaylor17 commented 1 year ago

Commit: d6d862c0f7f4beb37f654fbc5694a4a7f024c953

mablem8 commented 1 year ago

Everything in commit d6d862c0f7f4beb37f654fbc5694a4a7f024c953 looks good except the TODO on the com.c erase handler; use libopencm3 to erase the on-chip Flash of the NRF51 MCU.

When submitting an Issue fix, use the following format:

txcmd: bootloader_erase hw_id:0x1234 msg_id:0x0008 src:gnd dst:cdh
reply: bootloader_ack hw_id:0x1234 msg_id:0x0008 src:cdh dst:gnd reason:0x01(erased)
ChadTaylor17 commented 1 year ago

Commit: 516948768888a1e4898041f0a1e0ff605cb94dd5

mablem8 commented 1 year ago

The flash_unlock, flash_erase_page, flash_clear_status_flags, and flash_lock functions are not defined in libopencm3 for the NRF51 chip.

You’ll have to implement the erase handler differently than it’s implemented for the CDH board.

Some examples:

ChadTaylor17 commented 1 year ago

Added implementation of bootloader_erase for com, a bit slow. Take a look: fba7463941d75903d7ede66c0c31b5cc2f499320

mablem8 commented 1 year ago

There are a few issues preventing a merge of commit fba7463941d75903d7ede66c0c31b5cc2f499320.

Issues with the code (i.e., bugs)

There are some bugs in the proposed fix. For example, line 53 calculates an address for the start of each "bootloader write" sub-page. While the address is calculated correctly, it is then used as the address for the start of a page to erase at line 55.

The NRF51 reference manual specifies that the address supplied to ERASEALL should be an address of the first word in a page. The product specification states that a Flash page erase takes a maximum of 22.3 ms. Since the backwards-compatible "bootloader write" command supports 256 sub-pages of 128 bytes each, and since the NRF51 chip has a Flash page size of 1024 bytes, the bootloader_erase command should perform 32 different page erases. Instead, the proposed code performs 255 erases for a maximum potential erase time of nearly 5.7 seconds. A majority of these erases start at an address that does not correspond to the start of a Flash page, and the behavior of such erases is not specified in the reference manual.

Issues with licensing

Because the proposed fix directly leverages code from the nRF51 SDK examples, the code might fall under the custom Nordic Software Development Kit License agreement. Since it is unclear whether the potential restrictions of that agreement would conflict with the intended use of TAB, the code cannot be merged as-is.

Issues with workflow

Because the proposed commit also includes a proposed fix for Issue #21, the proposed fix to this issue cannot be tested independently from the code aimed at the other issue. If, for example, the fix for this issue passes but the fix for the other issue fails, then the commit would be prevented from being merged despite containing a working fix.

mablem8 commented 1 year ago

Commit 4352a768c71deef9ee70c78fd55c6473b5409f90 fixes the remaining open tasks in this issue and could use further testing.