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

LPC1768 IAP Fix #4993

Merged
merged 12 commits into from Sep 22, 2017

Conversation

Projects
None yet
4 participants
@chrissnow
Contributor

chrissnow commented Aug 30, 2017

Description

Reinstatates LPC1768 IAP by using HAL routines rather than CMSIS Algo.

Status

READY

+-------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
| target      | platform_name | test suite                   | test case                     | passed | failed | result | elapsed_time (sec) |
+-------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - buffer alignment test | 1      | 0      | OK     | 0.17               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - clock and cache test  | 1      | 0      | OK     | 0.06               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - erase sector          | 1      | 0      | OK     | 0.14               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - init                  | 1      | 0      | OK     | 0.06               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - mapping alignment     | 1      | 0      | OK     | 0.06               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - program page          | 1      | 0      | OK     | 0.26               |
+-------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
+-------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
| target      | platform_name | test suite                          | test case                 | passed | failed | result | elapsed_time (sec) |
+-------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - init           | 1      | 0      | OK     | 0.04               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - program        | 1      | 0      | OK     | 0.28               |
| LPC1768-ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - program errors | 1      | 0      | OK     | 0.06               |
+-------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Aug 30, 2017

GCC also passes.

+-----------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
| target          | platform_name | test suite                          | test case                 | passed | failed | result | elapsed_time (sec) |
+-----------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - init           | 1      | 0      | OK     | 0.04               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - program        | 1      | 0      | OK     | 0.28               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_drivers-flashiap | FlashIAP - program errors | 1      | 0      | OK     | 0.07               |
+-----------------+---------------+-------------------------------------+---------------------------+--------+--------+--------+--------------------+
+-----------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
| target          | platform_name | test suite                   | test case                     | passed | failed | result | elapsed_time (sec) |
+-----------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - buffer alignment test | 1      | 0      | OK     | 0.18               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - clock and cache test  | 1      | 0      | OK     | 0.08               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - erase sector          | 1      | 0      | OK     | 0.14               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - init                  | 1      | 0      | OK     | 0.07               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - mapping alignment     | 1      | 0      | OK     | 0.05               |
| LPC1768-GCC_ARM | LPC1768       | mbed-os-tests-mbed_hal-flash | Flash - program page          | 1      | 0      | OK     | 0.25               |
+-----------------+---------------+------------------------------+-------------------------------+--------+--------+--------+--------------------+
*/
unsigned long GetSecNum (unsigned long address) {
unsigned long n;

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

{ on t he new line, please leave one line space between functions (line 70)

uint8_t *data;
unsigned long n;
if ((unsigned long)datain%4==0)//word boundary

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

2 spaces instead of 4? Please read the above coding style and align this code

@0xc0170

Code style alingment

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2017

Thanks for fixing the previous bug in the implementation !

@0xc0170 0xc0170 added the needs: work label Aug 31, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@chrissnow bump

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 4, 2017

Code tidied but I still have a warning to sort out

mbed-os\targets\TARGET_NXP\TARGET_LPC176X\device\flash_api.c(117): warning:  #513-D: a value of type "const uint8_t *" cannot be assigned to an entity of type "uint8_t *"

I also want to understand SET_VALID_CODE and what we want it to be set to.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 4, 2017

@0xc0170 This should now be complete.

@0xc0170

Can you align if else code aligment , as shared previously to the mbed coding style please?

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 4, 2017

Apologies, I think that matches now.

free(alignedData);
}
if (IAP.stat) return (1);// Command Failed

This comment has been minimized.

@0xc0170

0xc0170 Sep 4, 2017

Member

Should be

if (IAP.stat) {
    return (1);
}
uint32_t flash_get_sector_size(const flash_t *obj, uint32_t address)
{
if(address < flash_get_start_address(obj) || address >= flash_get_start_address(obj) +flash_get_size(obj)){

This comment has been minimized.

@0xc0170

0xc0170 Sep 4, 2017

Member

if (address.....) (space after if)

* Return Value: Sector Number
*/
unsigned long GetSecNum (unsigned long address) {

This comment has been minimized.

@0xc0170

0xc0170 Sep 4, 2017

Member

{ new line

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 4, 2017

I think I now have eclipse set up to format to mbed style.

@0xc0170

0xc0170 approved these changes Sep 4, 2017

We will squash this as it comes as one fix (12 commits are mostly devel commits in here)

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 4, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 22, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1359

All builds and test passed!

@0xc0170 0xc0170 merged commit e2c42bb into ARMmbed:master Sep 22, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

@chrissnow we are looking at this again, seems like IAR fails from time to time for the flashiap test. Have you noticed any sporadic failures with program() ?

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 25, 2017

@0xc0170 I'm wondering if malloc isn't always assigning an aligned location like I expected.
C99 7.20.3

The pointer returned if the allocation
succeeds is suitably aligned so that it may be assigned to a pointer to any type of object

I didn't spend much time with IAR other than to get the test results a couple of times.

I haven't had any issues with ARM with my project though.

Do you know if its hard faulting? that's what will happen if it isn't aligned and I guess timeout the test.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

Do you know if its hard faulting? that's what will happen if it isn't aligned and I guess timeout the test.

I've attached debugger, it's hard faulting, I am investigating where it writes and how to get into the hardfault. Or maybe not hardfault at al,, will provide an update

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 25, 2017

I may have figured it out,
the IAR linker is missing the ram reservation for IAP

define symbol __ICFEDIT_region_RAM_end__ = 0x10007FDF;

I think that needs to be 0x10007FE0
see
; 32KB (RAM size) - 0xC8 (NIVT) - 32 (topmost 32 bytes used by IAP functions) = 0x7F18

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

Looks correct that RAM end is not defined as it should, I am however also running test with this fix (RAM end should be 0x10007EF8).

It fails sporadically. As it is, it gets to hardfault. With the fixed RAM end, debugging crashes under IAR (can't do anything just wait until I receive an error a target cant be stopped).
It happens during programming, at 2 addresses (I so far got 2 different addresses). Stacked PC counter indicates that is SystickHandler and fault register shows invalid instruction fault.

Some logs


Erasing: address 491520, sector size: 32768
Programming
Programming: page_addr 491520, page_size: 1024
Programming: page_addr 492544, page_size: 1024
Programming: page_addr 493568, page_size: 1024
Programming: page_addr 494592, page_size: 1024
Programming: page_addr 495616, page_size: 1024
Programming: page_addr 496640, page_size: 1024
Programming: page_addr 497664, page_size: 1024
Programming: page_addr 498688, page_size: 1024
Programming: page_addr 499712, page_size: 1024
Programming: page_addr 500736, page_size: 1024
Programming: page_addr 501760, page_size: 1024
Programming: page_addr 502784, page_size: 1024
Programming: page_addr 503808, page_size: 1024
Programming: page_addr 504832, page_size: 1024
Programming: page_addr 505856, page_size: 1024
Programming: page_addr 506880, page_size: 1024
Programming: page_addr 507904, page_size: 1024
Programming: page_addr 508928, page_size: 1024
Programming: page_addr 509952, page_size: 1024
Programming: page_addr 510976, page_size: 1024
Programming: page_addr 512000, page_size: 1024
Programming: page_addr 513024, page_size: 1024
Programming: page_addr 514048, page_size: 1024
Programming: page_addr 515072, page_size: 1024
Programming: page_addr 516096, page_size: 1024
Programming: page_addr 517120, page_size: 1024
Programming: page_addr 518144, page_size: 1024
Programming: page_addr 519168, page_size: 1024
Programming: page_addr 520192, page_size: 1024
Programming: page_addr 521216, page_size: 1024
Programming: page_addr 522240, page_size: 1024
Programming: page_addr 523264, page_size: 1024
Reading
Deinit
TEST DONE Done


Hard fault

Erasing: address 491520, sector size: 32768
Programming
Programming: page_addr 491520, page_size: 1024
Programming: page_addr 492544, page_size: 1024
Programming: page_addr 493568, page_size: 1024
Programming: page_addr 494592, page_size: 1024
Programming: page_addr 495616, page_size: 1024
Programming: page_addr 496640, page_size: 1024
Programming: page_addr 497664, page_size: 1024
Programming: page_addr 498688, page_size: 1024
Programming: page_addr 499712, page_size: 1024
Programming: page_addr 500736, page_size: 1024
Programming: page_addr 501760, page_size: 1024
Programming: page_addr 502784, page_size: 1024
Programming: page_addr 503808, page_size: 1024
Programming: page_addr 504832, page_size: 1024
Programming: page_addr 505856, page_size: 1024
Programming: page_addr 506880, page_size: 1024
Programming: page_addr 507904, page_size: 1024
Programming: page_addr 508928, page_size: 1024
Programming: page_addr 509952, page_size: 1024
Programming: page_addr 510976, page_size: 1024
Programming: page_addr 512000, page_size: 1024
Programming: page_addr 513024, page_size: 1024
Programming: page_addr 514048, page_size: 1024
Programming: page_addr 515072, page_size: 1024
Programming: page_addr 516096, page_size: 1024

Another hard fault

Erasing: address 491520, sector size: 32768
Programming
Programming: page_addr 491520, page_size: 1024
Programming: page_addr 492544, page_size: 1024
Programming: page_addr 493568, page_size: 1024
Programming: page_addr 494592, page_size: 1024
Programming: page_addr 495616, page_size: 1024
Programming: page_addr 496640, page_size: 1024
Programming: page_addr 497664, page_size: 1024
Programming: page_addr 498688, page_size: 1024
Programming: page_addr 499712, page_size: 1024
Programming: page_addr 500736, page_size: 1024
Programming: page_addr 501760, page_size: 1024


@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

@chrissnow Besides that RAM end that I'll fix, it seems that systick is ongoing (hardfault points to systick handler causing undefinstr error). If I suspend kernel (not needed for the test), all is running OK. I've run it at least 10x so far , all OK with my own test case.

Hints?

I'll rebuild everything, clean, and retest changes for RAM end again, to be certain that does not fix.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 25, 2017

Having done a bit of reading I think IAP blocks flash access during operations, which isn't going to end well with interrupts.

Perhaps disable interrupts during IAP operations?

https://os.mbed.com/questions/54146/LPC824-RTOS-and-IAP/

IAP is indeed a ROM call on LPC devices, but I don't know if the ROM code disables interrupts. For example I know for a fact that all flash access during programming/erasing of any sector results in the MCU crashing. It can handle a while(IAP_read) loop, most likely since that is prefetched from flash, for the LPC this is not relevant however since it is fetched from ROM. However if interrupts would not be disabled, it would jump to the interrupt vector where it would need to use the flash.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Sep 25, 2017

And from the LPC1768 user manual

32.3.2.6 Interrupts during IAP
The on-chip flash memory is not accessible during erase/write operations. When the user
application code starts executing the interrupt vectors from the user flash area are active.
The user should either disable interrupts, or ensure that user interrupt vectors are active in
RAM and that the interrupt handlers reside in RAM, before making a flash erase/write IAP
call. The IAP code does not use or disable interrupts.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

Run from new branch, all seems fine , sent a fix - #5189

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 25, 2017

@chrissnow Thanks, that is what I suspected. We shall add critical section then, lets take it to the fix, I can add one more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment