Clean up and extend RAMTRON driver #13621

nexton-winjeel · 2020-02-20T06:47:07Z

We experienced an issue where parameters were being unexpectedly erased during development. The sequence of events that caused this was:

The RAMTRON driver fails to initialise;
The Storage driver silently falls back to using the SD card;
The SD card was not fitted, so the Storage drive falls back to using Flash;
A new program image was written to the device over JTAG, erasing the params in Flash;
Developer tests the new image, and finds unexpected params.

This behaviour was observed on both a Pixracer and custom hardware.

The root cause of the the RAMTRON driver failing to initialise (step 1. above) was different data returned by different manufacturers. Fujitsu and Cypress make FRAM chips that are both pin and register compatible, but each manufacturer returns different data when the RDID register is read.

This pull request contains the following key changes to address the issues discovered during this investigation:

RAMTRON:
- Updated the struct that defines the supported FRAM devices to include the RDID format. This was then used to properly parse the read of the RDID register.
- Updated the init function to ensure the WriteEnable and BlockProtect bits of the status register are properly set.
- Updated the read and write functions so that the SPI read and write calls were inside the same chip select activation. This prevents spurious data being returned if the FRAM chip resets it's read registers on deselect.
- Updated the read and write functions to fail if we attempt to access more data than the FRAM chip can hold. Previously these would silently wrap back to address 0x0000, which would cause unexpected behaviour.
AP_HAL_ChibiOS/Storage:
- Added a field and extra logging to capture what physical device is being used by the Storage driver.
- Added a panic if no suitable backend device can be initialised by the Storage driver.
- Added an optional panic (intended to be controlled by a define in the hwdef.dat file) that prevents falling back to another device if the FRAM can't be initialised.

Please let me know if you need test evidence and/or test cases for this.

tridge · 2020-02-21T09:12:03Z

thanks for your work on this!
Note that there is some overlap with #13581, especially in removing the fallback when using FRAM

tridge

thanks for your work on this!
A few changes needed are listed below, plus I'd like to add the retries from #13581. That PR came out of a board where power up speed seems to cause the init of the ramtron to fail sometimes. I don't think the result is explained by your changes.

tridge · 2020-02-21T10:36:26Z

libraries/AP_Param/AP_Param.h

@@ -517,7 +517,8 @@ class AP_Param
        uint8_t magic[2];
        uint8_t revision;
        uint8_t spare;
-    };
+    } PACKED;


adding PACKED on this structure doesn't change the shape for storage, but does make g++ produce less efficient code (it makes for larger flash size for no advantage)

This is a habit from working with space constrained devices. Removed the PACKED attribute.

tridge · 2020-02-21T10:37:08Z

libraries/AP_Param/AP_Param.h

@@ -539,7 +540,8 @@ class AP_Param
        uint32_t type : 5;
        uint32_t key_high : 1;
        uint32_t group_element : 18;
-    };
+    } PACKED;


same here, PACKED just makes for less efficient code

Removed the PACKED attribute.

tridge · 2020-02-21T10:38:43Z

libraries/AP_RAMTRON/AP_RAMTRON.cpp

        uint8_t manufacturer[6];
        uint8_t memory;
        uint8_t id1;
        uint8_t id2;
-    } rdid;
-    if (!dev->read_registers(RAMTRON_RDID, (uint8_t *)&rdid, sizeof(rdid))) {
+    } PACKED;


PACKED doesn't help here and makes for slower and larger code

Removed the PACKED attribute. Replaced with a static_assert to ensure the RDID shape is correct.

tridge · 2020-02-21T10:38:54Z

libraries/AP_RAMTRON/AP_RAMTRON.cpp

+        uint8_t manufacturer[2];
+        uint8_t id1;
+        uint8_t id2;
+    } PACKED;


PACKED doesn't help

As above, removed PACKED attribute, added static_assert.

tridge · 2020-02-22T04:59:05Z

ArduPlane/Parameters.cpp

@@ -45,7 +45,7 @@ const AP_Param::Info Plane::var_info[] = {
    ASCALAR(autotune_level, "AUTOTUNE_LEVEL",  6),

    // @Param: TELEM_DELAY
-    // @DisplayName: Telemetry startup delay 


could you see if you can configure your editor not to do whitespace cleanup on lines you don't edit?
You may find using "git add -p" to build a patchset useful.

Ack. Once you're happy with the changes, I'll rebase and remove the whitespace changes.

Are you aware that you can append ?w=1 to the URL to hide whitespace changes in the diff? Maybe you can do this in the interim?

yes, I'm aware. It just pollutes commits however, changing the git blame results

Rebased to remove the whitepace changes.

tridge · 2020-02-22T05:04:38Z

libraries/AP_RAMTRON/AP_RAMTRON.h


    // read from device
-    bool read(uint32_t offset, uint8_t *buf, uint32_t size);
+    uint32_t read(uint32_t offset, uint8_t * const buf, uint32_t size);


why change from bool when the only use case is to check if we get the full amount requested?

It was useful while I was debugging to see why I wasn't getting back the data I expected. It also allows consumers to do partial retries.

Would you prefer this reverted back to returning a bool?

yes, I'd prefer it be bool. If we end up with a consumer that wants to do partial retries in the future we could change this, but I doubt we will

Reverted to bool.

tridge · 2020-02-22T05:04:50Z

libraries/AP_RAMTRON/AP_RAMTRON.h


    // write to device
-    bool write(uint32_t offset, const uint8_t *buf, uint32_t size);
+    uint32_t write(uint32_t offset, uint8_t const * const buf, uint32_t size);


why change from bool?

As above...

tridge · 2020-02-22T05:11:36Z

libraries/AP_RAMTRON/AP_RAMTRON.cpp

-    // get data
-    dev->transfer(nullptr, 0, buf, size);
+            uint32_t const kReadSz = MIN(size, kMaxReadSz);
+            bool ok = dev->set_chip_select(true);


transfer() already handles chip select, so is only needed when doing multiple operations within the one CS op

Removed the unnecessary calls to set_chip_select().

tridge · 2020-02-22T05:26:24Z

libraries/AP_HAL_ChibiOS/Storage.cpp

+        if (fram.read(0, _buffer, CH_STORAGE_SIZE) == CH_STORAGE_SIZE) {
+            _save_backup();
+            _initialisedType = StorageBackend::FRAM;
+            hal.console->printf("Initialised Storage type=%d\n", _initialisedType);


this happens so early that it isn't visible on the console as console isn't setup yet

Changed back to using ::printf()

tridge · 2020-02-22T05:27:53Z

libraries/AP_HAL_ChibiOS/Storage.cpp

+        if (sdcard_retry()) {
+            log_fd = AP::FS().open(HAL_STORAGE_FILE, O_RDWR|O_CREAT);
+            if (log_fd == -1) {
+                hal.console->printf("open failed of " HAL_STORAGE_FILE "\n");


::printf() was used as it works in early startup

Ack. As above, reverted to use ::printf()

Jaaaky · 2020-02-23T13:32:55Z

I've 3 fmuv3 boards, pixhawk1 like, which do not save any parameters.
The firmware doesn't fallback to sdcard as - I think - it can see an FRAM, so any param change is lost on reboot.
I really think it's a driver issue, but not sure.
I've tested this pull request and @tridge's one, but no success.

On this pull request it gives "Couldn't load params: erasing params..."
I can debug it further, if you tell me how.

nexton-winjeel · 2020-02-24T04:41:21Z

Please note: The latest changes compile, but I won't have a chance to test them on the relevant hardware until tomorrow.

andyp1per · 2020-02-24T08:56:48Z

libraries/AP_HAL_ChibiOS/Storage.cpp

    }
+
+    #if defined(HAL_RAMTRON_ALLOW_FALLBACK) && !HAL_RAMTRON_ALLOW_FALLBACK
+        AP_HAL::panic("Unable to init RAMTRON storage");


Looks like your indentation has gone wonky here

andyp1per · 2020-02-24T09:00:00Z

libraries/AP_HAL_ChibiOS/Storage.h

@@ -48,7 +48,13 @@ class ChibiOS::Storage : public AP_HAL::Storage {
    bool healthy(void) override;

 private:
-    volatile bool _initialised;
+    enum StorageBackend {


andyp1per · 2020-02-24T09:00:44Z

libraries/AP_HAL_ChibiOS/Storage.h

+        Flash,
+        SDCard,
+    };
+    volatile enum StorageBackend _initialisedType = StorageBackend::None;


don't think you need the enum here

Don't need the enum now that StorageBackend is an enum class. Fixed.

rmackay9 · 2020-03-01T23:30:42Z

We had another report in the forums of this happening. It very often seems to happen as part of a firmware upgrade. https://discuss.ardupilot.org/t/all-parameters-set-to-default-by-themselves/52983

While they share the same register maps, the FRAM devices from Cypress and Fujitsu return a different data structure when the RDID register is read.

This driver will panic if HAL_RAMTRON_NO_FALLBACK is defined in the hwdef.dat file and the RAMTRON initialisation fails. This prevents us from silently falling back to a potentially different parameter set and causing unexpected behaviour.

…is used Note: This usually won't get seen as the USB is not up when this is printed. However, adding a delay (of ~2s) into the main_loop() function in the HAL_ChibiOS_Class.cpp, just after the scheduler is initialised, will allow this message to be seen.

…efined This #error pragma causes the Bootloader to fail, even though this file class isn't used by the Bootloader. However, it is included in the base AP_HAL definition that the Bootloader pulls in.

…e bit

Inline initialisation of VLAs will cause a compilation error under Clang, as it is not allowed according to the C specification. However, GCC will allow it.

::printf() is available earlier than hal.console->printf() during boot.

Change from HAL_RAMTRON_NO_FALLBACK to HAL_RAMTRON_ALLOW_FALLBACK.

This adds equivalent functionality that was originally added in: ArduPilot#13581

nexton-winjeel · 2020-03-06T02:06:45Z

thanks for your work on this!
A few changes needed are listed below, plus I'd like to add the retries from #13581. That PR came out of a board where power up speed seems to cause the init of the ramtron to fail sometimes. I don't think the result is explained by your changes.

@tridge: I've incorporated equivalent changes to #13581 in commits 2dc533a and 528fe35.

nexton-winjeel · 2020-03-06T02:07:43Z

Tidied up and rebased onto latest master.

tridge · 2020-03-06T21:45:04Z

@sypaq-nexton the main holdup on this PR now is the investigation we have going on into the param resets on cube orange. The changes you have done unfortunately don't explain the issue. We're seeing params reset with a firmware that only has RAMTRON storage enabled, and with a firmware that does every read twice to confirm with CRC. Most bizarrely we're seeing resets even when we completely remove the write() code in the ramtron driver, so it should have no ability to write to the ramtron at all.
My current theory is that the reset it actually happening in the bootloader. The CS line is currently set to floating in the bootloader, and the reset seems to happen when powering the cube with particularly type of power system (eg. certain type of ESC setup). So I'm wondering if the floating CS combined with some electrical noise in the early startup of the MCU could make the FRAM think that it is getting a write transaction while still in the bootloader. We're getting the user who can reproduce this to test a new bootloader that forces the CS line high while in the bootloader

tridge · 2020-03-10T21:37:13Z

@sypaq-nexton we've now proven that CS pin floating in the bootloader is the cause of the resets that have particulatly plagued cube orange. We will still do the improvements to the RAMTRON driver, but the key fix seems to be this one: #13780

tridge · 2020-03-11T03:42:54Z

@sypaq-nexton I did tests of this branch on a CubeBlack and found that parameters did not persist. The change of approach for the latching of the write enable bit of the FRAM doesn't work on that cypress chip. It is also dangerous as it leaves write enabled as the normal state when flying which means that on reboot the floating CS pin is much more likely to cause corruption.
I have now updated #13581 to include the other key changes from your PR and consolidated ready to merge. I will close this PR and get that one merged.

nexton-winjeel · 2020-03-11T22:07:24Z

Great, thanks @tridge!

nexton-winjeel requested a review from tridge February 20, 2020 06:48

khancyr added BUG ChibiOS DevCallTopic labels Feb 20, 2020

auturgy mentioned this pull request Feb 21, 2020

Parameters reset to default values #13134

Closed

tridge requested changes Feb 22, 2020

View reviewed changes

andyp1per reviewed Feb 24, 2020

View reviewed changes

CraigElder removed the DevCallTopic label Feb 24, 2020

rmackay9 added the DevCallTopic label Mar 1, 2020

CraigElder removed the DevCallTopic label Mar 3, 2020

nexton-winjeel added 14 commits March 6, 2020 10:59

AP_RAMTRON: Interpret RDID reg based on manufacturer

ce15de4

While they share the same register maps, the FRAM devices from Cypress and Fujitsu return a different data structure when the RDID register is read.

AP_RAMTRON: Get RAMTRON driver working

df07aec

AP_HAL_ChibiOS: Use updated RAMTRON driver

1d7e3f9

AP_Param: Print bad header values if setup() fails

7b6b4ba

AP_Param: Add static assert on NVM struct sizes

95abc66

AP_RAMTRON: Ensure RAMTRON write enable fields are set correctly

18299f6

AP_HAL_ChibiOS: Record which storage backend is in use

598376c

AP_HAL_ChibiOS: Remove #error pragma for when no storage backend is d…

bd1f132

…efined This #error pragma causes the Bootloader to fail, even though this file class isn't used by the Bootloader. However, it is included in the base AP_HAL definition that the Bootloader pulls in.

AP_RAMTRON: Ensure status register write doesn't overwrite WriteEnabl…

b3d2ec3

…e bit

AP_RAMTRON: Tidy up const correctness

aa373d2

AP_RAMTRON: Use enum class for RDID_type enum

03105c6

AP_HAL_ChibiOS: Remove inline initialisation of VLAs

ff29e83

Inline initialisation of VLAs will cause a compilation error under Clang, as it is not allowed according to the C specification. However, GCC will allow it.

nexton-winjeel added 12 commits March 6, 2020 12:50

AP_HAL_ChibiOS: Use ::printf() rather than HAL console

9855f26

::printf() is available earlier than hal.console->printf() during boot.

AP_HAL_ChibiOS: Invert naming and logic of FALLBACK define

a149a76

Change from HAL_RAMTRON_NO_FALLBACK to HAL_RAMTRON_ALLOW_FALLBACK.

AP_RAMTRON: Revert read() and write() to return bool

eb80ffe

AP_HAL_ChibiOS: Use updated RAMTRON driver read() and write()

84008d9

AP_RAMTRON: Clarify chip select settings for reads and writes

d54fff0

Plane: Add better logging to explain why parameters are being erased

1b00b26

Rover: Add better logging to explain why parameters are being erased

c7aac16

Tracker: Add better logging to explain why parameters are being erased

09bbb92

Copter: Add better logging to explain why parameters are being erased

6042ac8

Sub: Add better logging to explain why parameters are being erased

77aea19

AP_RAMTRON: Add retries to init()

2dc533a

This adds equivalent functionality that was originally added in: ArduPilot#13581

AP_RAMTRON: Add retries to read()

528fe35

This adds equivalent functionality that was originally added in: ArduPilot#13581

nexton-winjeel force-pushed the ramtron branch from 086eaf5 to 528fe35 Compare March 6, 2020 01:58

nexton-winjeel requested a review from tridge March 6, 2020 02:20

tridge mentioned this pull request Mar 10, 2020

Hold CS pins high in bootloader - fixes param reset issue #13780

Merged

tridge closed this Mar 11, 2020

tridge mentioned this pull request Mar 11, 2020

Added retries to FRAM init and read #13581

Merged

Clean up and extend RAMTRON driver #13621

Clean up and extend RAMTRON driver #13621

Conversation

nexton-winjeel commented Feb 20, 2020

tridge commented Feb 21, 2020

tridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaaaky commented Feb 23, 2020

nexton-winjeel commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 commented Mar 1, 2020

nexton-winjeel commented Mar 6, 2020 • edited

nexton-winjeel commented Mar 6, 2020

tridge commented Mar 6, 2020

tridge commented Mar 10, 2020

tridge commented Mar 11, 2020

nexton-winjeel commented Mar 11, 2020

nexton-winjeel commented Mar 6, 2020 •

edited