Add SAMD51 compatibility

[mitchellpontague]

Add preprocessor directives to replace chip registers with device specific alternatives.

[smarpug]

Add preprocessor directives to replace chip registers with device specific alternatives.

Is the library currently working for you on the SAMD51 with these changes, or are there further changes you know of that need to be made?

[BrunDog]

It didn't work for me... that said, I don't really know how to use Github. I'm not sure how to download the correct commits - does the last one include all the changes?

Yeah I don't think it is working... code that works on the SAMD21 doesn't on the SAMD51 (Adafruit Metro M4).

[BrunDog]

I double checked I pulled in all 3 commits... but no joy. For the record, here is the code I am trying, on an M4 Metro:

#include <FlashStorage.h>
 int number;
FlashStorage(my_flash_store, int);

void setup() {
  Serial.begin(115200);
}

void loop() {
  number = my_flash_store.read();

  Serial.print("Number: ");
  Serial.println(number);

  my_flash_store.write(number + 1);
  delay(2000);
}

[smarpug]

BrunDog, that's a valid test. You are probably aware of this, but I'd be careful running it too long, those flash bytes are only good for 10,000 write cycles or so. A better test would be to read the flash number and modify the number in setup, so it increments every time you power the unit on. This also makes sure you aren't incrementing a value in RAM, since incrementing the number is only possible if the number is, in fact, getting stored in non-volatile flash memory that persists while power is off.

I'll try and dig into this library this week and see if I can push this along with some pull requests of my own.

[BrunDog]

Yes, thanks. I don't run it long and my hardware is for the bench and meant to be worn out.

FWIW... changing the 'int' declaration to 'uint16_t' gives better results... so clearly there is a data width mismatch. And by better results, the above produces 1, 2, 3, 3, 4, 5, 6, 6, 7, 8, 9, 9, etc.

[mitchellpontague]

@BrunDog the easiest way i know to get the correct version is to just download or pull the branch that the pull request is based on https://github.com/mitchellpontague/FlashStorage

Also i'm not having the same data width or the doubled up numbers issue you are seeing. Below is using 'int' on a Metro M4

Number: 0
Number: 1
Number: 2
Number: 3
Number: 4
Number: 5
Number: 6
Number: 7

[cmaglie]

@BrunDog the easiest way i know to get the correct version is to just download or pull the branch that the pull request is based on https://github.com/mitchellpontague/FlashStorage

From the comments above is not clear to me if this patch working? If someone can confirm that is working I'm more than happy to merge and release a new verision.

[mitchellpontague]

@cmaglie It is working for me, was hoping someone from #21 could confirm it's working for them also.

[mitchellpontague]

Never mind, the samd21 uses 16 bit addressing for row erase. Where as in samd51 i can just use the pointer directly to address the block.

[BrunDog]

Just confirming... tested it again and getting this output with the last pull request:

Declaring number as an int, I get:
Number: 1
Number: 2
Number: -1
Number: -1
Number: -1
Number: -1
Number: -1
Number: -1
Number: -1
Number: -1
Number: -1

Using uint16_t, here is the output:

Number: 0
Number: 1
Number: 2
Number: 2
Number: 3
Number: 3
Number: 4
Number: 5
Number: 6
Number: 6
Number: 7
Number: 7
Number: 8
Number: 8
Number: 9
Number: 9
Number: 10
Number: 10

I have an Adafruit Metro M4 express.

[BrunDog]

OK... figured it out. The library works correctly when the processor cache is disabled. With it enabled, it gives the results above.

I would assume the memory instructions in the library are allowing the data to sit in the cache rather than getting committed. I would assume the chip runs faster with cache enabled, so we would probably want to see if its possible to force the cache to commit the write. Anyway... progress!

[mitchellpontague]

@BrunDog Could you confirm what version of Adafruit SAMD Boards you are using?

[BrunDog]

@BrunDog Could you confirm what version of Adafruit SAMD Boards you are using?

Just confirming... did you see my above comments? Disabling cache in the board options fixes the problem. I am using an Adafruit Metro M4 Express.

[mitchellpontague]

I did, I'm not super familiar with how the cache works, but i have seen some setup for it in Adafruit's Arduino core. So its possible different versions from the boards manager have different results.

[BrunDog]

What board are you using? Do you see the cache enable/disable option in the Tools menu?

I have Arduino SAMD v1.6.20 and Adafruit SAMD v1.2.9 installed. On Arduino IDE 1.8.5.

[mitchellpontague]

I'm using the Metro M4 express Beta, I've tried compiling with both cache enable and disable in the tools menu, but they both work for me. Same versions for everything as you.

[BrunDog]

Odd. My board is marked beta too, so it sounds like identical HW & SW. Not sure, but I am sure that enabling the cache causes it not to work correctly on mine.

[orhanyor]

so whats the verdict guys? does it work? im about to buy M4 but if this library is not working with it i will stick with M0.

[jacky4566]

Bump. Seems like mitchellpontague's code is working at least for me on the M4 Metro. Perhaps we can merge this into a dev branch.

[BrunDog]

Bump. Seems like mitchellpontague's code is working at least for me on the M4 Metro. Perhaps we can merge this into a dev branch.

Did you test it with cache enabled? I haven't tried it recently but testing across both the Adafruit Metro M4 and Grand Central M4 yielded the same results for me a while ago: works with cache disabled but doesn't with cache enabled.

[jacky4566]

@BrunDog It seems to work with and without cache. Here is the code i used to test:

#include <FlashStorage.h>

FlashStorage(my_flash_store, int);

void setup() {
  while (!Serial) {}

  int number = 0;
  number = my_flash_store.read();

  for (int i = 0; i < 10; i++) {
    Serial.print("Number: ");
    Serial.println(number);
    number++;
    my_flash_store.write(number);
    delay(1000);
  }
}

void loop() {
  __WFI();
}

And this is the output

Number: 0
Number: 1
Number: 2
Number: 3
Number: 4
Number: 5
Number: 6
Number: 7
Number: 8
Number: 9

[orhanyor]

@BrunDog It seems to work with and without cache. Here is the code i used to test:

#include <FlashStorage.h>

FlashStorage(my_flash_store, int);

void setup() {
  while (!Serial) {}

  int number = 0;
  number = my_flash_store.read();

  for (int i = 0; i < 10; i++) {
    Serial.print("Number: ");
    Serial.println(number);
    number++;
    my_flash_store.write(number);
    delay(1000);
  }
}

void loop() {
  __WFI();
}

And this is the output

Number: 0
Number: 1
Number: 2
Number: 3
Number: 4
Number: 5
Number: 6
Number: 7
Number: 8
Number: 9

tried the same code i got the same result with cache enabled, so i suppose everything works as it should.

also tried this code for float and bigger int number and again no problems

 #include<FlashStorage.h>

FlashStorage(flash, float);
FlashStorage(flashin, int);
void setup() {
  while (!Serial) {}

  float number;
  int number1;
  flash.write(456.32);
  flashin.write(563);
  number = flash.read();
  number1 = flashin.read();



  Serial.print("Number: ");
  Serial.println(number);
  Serial.print("Number1: ");
  Serial.println(number1);

  delay(1000);
}


void loop() {
  __WFI();
}

my arduino is the latest version also feather m4 is with the latest bootloader installed.

[orhanyor]

tested the code with samd21 as well so the one for samd51 also works ok for samd21. i was switching back and forth between the two version of libraries so i guess theres no need to.

[cmaglie]

I see that arduino/Arduino#9956 a new library has been proposed.
I know I've been a bit out-of-the-loop, but it's a pity to spread dev energy among many forks.

So my question is: there are any blocking to this one? can we merge it?
At least it should not break compatibility with SAMD21 so it should be safe to merge anyway?

There is someone else willing to help maintain the library? I'm positive to give write access to the repo if needed.

[dhalbert]

@cmaglie: @ladyada asked me to take a look this: I've been some SAMD51 flash work recently. @khoih-prog : I can compare this to https://github.com/khoih-prog/FlashStorage_SAMD too. I'm not sure what direction this should go in: that new library or a PR to here?

[mitchellpontague]

@cmaglie I'm confident that this patch is working and is safe to merge.

[cmaglie]

I've compared the changes made by @khoih-prog, they are basically the same as this PR except for the piece:

  // Disable automatic page write
  NVMCTRL->CTRLB.bit.MANW = 1;

where he added a check for the READY status:

  // Disable automatic page write
  NVMCTRL->CTRLA.bit.WMODE = 0;
  while (NVMCTRL->STATUS.bit.READY != NVMCTRL_STATUS_READY ) { } 

@mitchellpontague what do you think? This may explain the "cache" issue @BrunDog was experiencing? in case may you add the same check to the PR? for reference @khoih-prog implementation is here: https://github.com/khoih-prog/FlashStorage_SAMD/blob/master/src/FlashStorage_SAMD51.h#L74

@dhalbert if you can test this PR (with and without the READY check above) and confirm it works it would be great!

[cmaglie]

Ok, since the changes of this PR and the @khoih-prog are the same and we had many positive feedback. I've moved forward and merged it.

I guess that we only need to discuss about the READY status bit above. I'll open a PR shortly about this.