Should I read directly from QSPI?

Is it necessary to copy a variable value from QSPI to RAM as opposed to just reading/writing it directly from QSPI? E.g. is this:

uint8_t DSY_QSPI_BSS foo_qspi[1];
uint8_t foo_ram[1];

int main(void){
    // memcpy foo_qspi to foo_ram

    // do stuff with foo_ram

    // memcpy foo_ram to foo_qspi whenever a save is necessary
}

Better than this?

uint8_t DSY_QSPI_BSS foo_qspi[1];

int main(void){
    // do stuff directly with foo_qspi
}

Thank you!

For basic cases you can certainly read directly from the QSPI – However, there are a few cases where it can result in a crash that would be otherwise hard to diagnose:

  • If you’re accessing the data in an audio callback (an interrupt event), it may happen while the QSPI is configured for writing. In which case, the device will fault when attempting to access that address.
  • If you try to access that address before initializing the QSPI (done within the hardware Init) the device will fault.

So if you plan on reading and writing within the application, it is probably a good idea to keep a local copy of the data. That said, if you’re storing large lookup tables that don’t change, this may not be feasible, or required.

The PersistentStorage class manages storing data between power cycles, and keeps a local copy of the data in RAM. You can use that class if it fits your use case, or look at it’s source code as a jumping off point for a custom solution.

Also, looking at your above code snippet, its important to note that the QSPI memory section is read-only – so attempting to write to it will not work as expected. You, instead need to use the QSPIHandle::Write function, which switches the device mode, and writes the data to the QSPI (this is all handled within the PersistentStorage class linked above.

Hope that answers your question! Let me know if you have any questions :slight_smile:

Sorry to hijack the conversation, but I think this may be useful.
I’m trying to implement PersistentStorage in my Versio project but I’m not really understanding how to do it. Here’s what I have so far:

QSPIHandle qspi;
QSPIHandle::Config qspi_config;

struct Settings
{
    float a; // Example
};

Settings settings{1.f}; // Default values

PersistentStorage<Settings> storage(qspi);

bool operator!=(const Settings& lhs, const Settings& rhs)
{
    return lhs.a != rhs.a; // Example
}

Settings& operator* (const Settings& settings) { return *settings; }

void Init()
{
    ... // Other init stuff

    qspi_config.device = QSPIHandle::Config::Device::IS25LP064A;
    qspi_config.mode   = QSPIHandle::Config::Mode::MEMORY_MAPPED;

    qspi_config.pin_config.io0 = dsy_pin(DSY_GPIOF, 8);
    qspi_config.pin_config.io1 = dsy_pin(DSY_GPIOF, 9);
    qspi_config.pin_config.io2 = dsy_pin(DSY_GPIOF, 7);
    qspi_config.pin_config.io3 = dsy_pin(DSY_GPIOF, 6);
    qspi_config.pin_config.clk = dsy_pin(DSY_GPIOF, 10);
    qspi_config.pin_config.ncs = dsy_pin(DSY_GPIOG, 6);
    qspi.Init(qspi_config);

    storage.Init(settings);
}

Later, when the “a” parameter changes:

settings.a = 0.5f; // Example
storage.Save();

The firmware compiles and flashes without problems, but the settings are not persisted.

I also tried loading the defaults value with

storage.RestoreDefaults();

but it just crashes the module.

Am I terribly off track here?

Thanks!

What you have looks okay at a glance.

However, during DaisySeed::Init the DaisySeed::QSPIHandle object is set up. So unless you’re not using the seed, or versio class, then that could be causing a slight issue.

So I would recommend:

DaisySeed hw;
PersistentStorage<Settings> storage(hw.qspi);

when constructing your storage struct.

Where in your program are you loading settings.a to check if it restored?

Also, where in your program are you calling the Save and RestoreDefaults functions. If you’re attempting to call them from within the audio callback that could have unintended results (including crashing).

I’m using DaisyVersio, so I can indeed use:

PersistentStorage<Settings> storage(hw.seed.qspi);

Where in your program are you loading settings.a to check if it restored?

Also, where in your program are you calling the Save and RestoreDefaults functions. If you’re attempting to call them from within the audio callback that could have unintended results (including crashing).

Well, I’m reading the struct and saving the storage in the audio callback, but that doesn’t seem to cause any crash. But maybe the apparent lack of persistence is a sign.

What is always crashing the module, though, is calling RestoreDefaults(), but I do this before setting the audio callback and just after calling

storage.Init(settings);

I try to extract anything I can from the audio callback and see if it solves.

A doubt: using DSY_QSPI_BSS is not necessary, right?

Thanks for the support!

Ah, you can read the settings from the audio callback, but saving should take place in the main while loop.

The save itself can take quite a while (multiple milliseconds), and internally uses Delay which cannot be used within the Audio Callback without causing issues.
So I expect that moving the Save() function itself out of the main loop will get it working!

What I normally do is trigger the save from the audio callback, and then periodically run the save from the while loop if the flag has been set (every 5-10 seconds, or on a specific UI action to prevent saving too often).

Here’s a brief snippet. The trigger_save could be set by a specific button press, combination, etc. if you want, or omitted if you just want to save periodically.

// in main before while()
uint32_t last_save_time = System::GetNow(); 

// In main while loop:
if (System::GetNow() - last_save_time > 10000 && trigger_save)
{
  storage.Save();
  last_save_time = System::GetNow();
  trigger_save = false;
}

I’m not really sure why RestoreDefaults would crash, since it should just do a generic copy on your structure.

I just checked, and there isn’t a unit test for the RestoreDefaults function. I’m pretty sure I’ve used it a bit, but there could just be an issue with that function that never got caught.

Hope that helps! :slight_smile:

Thanks, I’ve implemented something similar, but I don’t think is either saving or recalling.

Here is a minimal case that hopefully can help:

#include "daisy_versio.h"

using namespace daisy;

DaisyVersio hw;

struct Settings
{
    float knob1;
    float knob2;
};

Settings localSettings{};
Settings settings{};

PersistentStorage<Settings> storage(hw.seed.qspi);

bool operator!=(const Settings& lhs, const Settings& rhs)
{
    return lhs.knob1 != rhs.knob1 || lhs.knob2 != rhs.knob2;
}

Settings& operator* (const Settings& settings) { return *settings; }

bool startUp{true};
bool mustUpdateStorage{};

void AudioCallback(AudioHandle::InputBuffer in, AudioHandle::OutputBuffer out, size_t size)
{
    hw.ProcessAnalogControls();
    hw.tap.Debounce();

    if (startUp)
    {
        startUp = false;

        // Trying to copy settings from the stored ones, but always getting the
        // an empty struct.
        localSettings.knob1 = settings.knob1;
        localSettings.knob2 = settings.knob2;

        return;
    }

    // Handle knobs and settings updates.
    if (std::abs(hw.knobs[DaisyVersio::KNOB_0].Value() - localSettings.knob1) >= 0.002f)
    {
        settings.knob1 = localSettings.knob1 = hw.knobs[DaisyVersio::KNOB_0].Value();
        mustUpdateStorage = true;
    }
    if (std::abs(hw.knobs[DaisyVersio::KNOB_1].Value() - localSettings.knob2) >= 0.002f)
    {
        settings.knob2 = localSettings.knob2 = hw.knobs[DaisyVersio::KNOB_1].Value();
        mustUpdateStorage = true;
    }

    for (size_t i = 0; i < size; i++)
    {
        float leftIn{IN_L[i]};
        float rightIn{IN_R[i]};

        float leftOut = leftIn;
        float rightOut = rightIn;

        OUT_L[i] = leftOut;
        OUT_R[i] = rightOut;
    }
}

int main(void)
{
    hw.Init(true);
    hw.StartAdc();

    storage.Init(settings);

    // This crashes the module:
    storage.RestoreDefaults();
    // Dies here:
    // if(SCB->CFSR & SCB_CFSR_IMPRECISERR_Msk)
    //     __asm("BKPT #0");

    hw.StartAudio(AudioCallback);

    while (1)
    {
        // Here I save the storage, if flagged to do so.
        if (mustUpdateStorage)
        {
            storage.Save();
            mustUpdateStorage = false;
        }
    }
}

OK, I did a bit of digging, and was working on a project of my own that uses the class and found what I think is the issue!

I was basing some of my previous answers on an earlier version of the class that operated on a reference to the “default” state struct that gets passed in via the Init function.

That struct only gets used to copy the defaults when initializing the class.

Once the class is initialized, it uses an internal structure that you can access via GetSettings

So the work flow may look a bit more like:

// Default instance via constructor or manual settings.
Settings my_default_settings;
storage.Init(my_settings);

// This can be created in main and maintained there, 
// or can be accessed elsewhere (i.e. AudioCallback)
Settings &my_real_settings = storage.GetSettings();

// then you can do stuff:
my_real_settings.knob1 = 0.2;

// and save it:
storage.Save();

Also, looking at the implementation I think the RestoreDefaultSettings is still set up for when it did work on a reference to the struct passed in as it’s trying to set the pointer value. This is likely causing the crash.

I’ve opened a github issue for this, and we should have a fix up soon.

Wonderful, I’ll try it later and let you know :slight_smile:

Thanks!

1 Like

Ok, it worked! The only difference is that, in my case, I couldn’t define and read the structure in the same scope, so I had to use a third global structure to temporarily store the values and then copy it to the referenced structure and save it in another scope.

I also confirm that changing

*settings_ = default_settings_;

to

settings_ = default_settings_;

in RestoreDefaults() fixes the crash.

Awesome! Glad you were able to get it working!

Hmm, you shouldn’t have to have a global for that, though if that works you can do it.

That said, you would have to create a new reference in each scope you want to use it in, but since it is just a reference and not a full copy of the struct being stored, this is actually a pretty cheap operation.

Normally, I’m only using that struct in the main() function when I load the settings initially, and when I collect data from other modules to actually perform the save function.

It just seemed cleaner to me to use a global structure instead of defining the same reference everywhere.
Thanks again for the support, I hope to present my patch here soon :slight_smile: