Skip to content
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

EEPROM -> Preferences for ESP32 #137

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

davy39
Copy link
Contributor

@davy39 davy39 commented Mar 3, 2024

For ESP32 : implement Preferences for settings storage as it "should be considered as the replacement for the Arduino EEPROM library".


protected:
int Fs;
EEPROMClass eeprom_settings;
Preferences preferences_settings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just call it settings?

@@ -226,8 +241,7 @@ void Edrumulus_hardware::setup ( const int conf_Fs,
{
// set essential parameters
Fs = conf_Fs;
eeprom_settings.begin ( ( number_pads + 1 ) * MAX_NUM_SET_PER_PAD ); // "+ 1" for pad-independent global settings

preferences_settings.begin ( "Settings", false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define Settings string in a constant

@davy39
Copy link
Contributor Author

davy39 commented Mar 4, 2024

Thanks, I use to code with python... I guess I need to learn to define and type constants !
Is it OK now ?

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Mar 4, 2024

Could you approve this workflow message @corrados ? Maybe these workflows approvals should be approved automatically?

afbeelding

@corrados
Copy link
Owner

corrados commented Mar 5, 2024

I changed something in the settings. So, hopefully now it works without the need for approval.

Copy link
Contributor

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot test it at the moment but changes look ok to me.

@corrados
Copy link
Owner

corrados commented Mar 9, 2024

Just to give you a bit of history regarding settings storage on the ESP32:

As I pointed out, I already had an implementation ready for this which seemed to work perfectly. So, I used this for my Protoype 3 (HD-1) which I used regularly. I could play for days without any issues. Then suddenly, one pad did not work anymore. I thought that this would be an electrical contact issue of some sort and did investigating the hardware. This process took me hours and finally I found out that simply the MIDI note in the Edrumulus settings had suddenly changed to a different value. That caused that no sound came out of the headphones for this particular pad. So, in the end, it was not a hardware issue but a settings issue. That was very annoying for me (and would be for other Edrumulus users as well).

Therefore, I am very cautious about settings storage on the ESP32. You are using a different library than I did which may solve that issue. But it may also be the case that internally in the library you now use, the same ESP functions are used which I used in the past.

Therefore, before I merge this code, I need you (and maybe @thijstriemstra) to extensively test it over a couple of days (maybe even weeks) to really make sure that it works reliable.

@corrados
Copy link
Owner

corrados commented Mar 9, 2024

BTW, the problem I see here is that you are using it only on the ESP32-S3 but I saw the problems when I was testing it with the older ESP32. Therefore, it may work for the S3 but not for the old ESP32.
These are problems you have if you support multiple different types of hardware. Testing and verifying can be a nightmare...

@thijstriemstra
Copy link
Contributor

You are using a different library than I did which may solve that issue.

The Preferences classes are part of official esp32 codebase (it's not a 3rd party library) and should be very stable (at least in my projects).

It'll need some testing though.

@davy39
Copy link
Contributor Author

davy39 commented Mar 9, 2024

I agree, but I'm not yet with a setup that could be used daily for extensive tests... I'll need to build the hardware first, but for now I'm trying to make the algorithms work with my mps850 pads #138 . Not easy to understand how to tune each parameters...

@corrados
Copy link
Owner

corrados commented Mar 9, 2024

I would start with playing with the existing pad types. Maybe you find some which give you good results. But not only play with the pad types but also with threshold, sense, etc. parameters

@corrados
Copy link
Owner

It's a while ago since you submitted this code. Have you had time to test it a lot? Did it work reliably for you?

@davy39
Copy link
Contributor Author

davy39 commented Apr 18, 2024

Hi @corrados ,
Sorry, I didn't have much time to work with edrumulus yet.
I only have a test hardware with 2 inputs.
For now, I've tried in vain to adjust both resistors and parameters to work on my positional sensing issue #139.
I hope I'll have more time to play with that next weeks.
Did someone else try this Preferences fork ?

@corrados
Copy link
Owner

Sorry, I didn't have much time to work with edrumulus yet.

Absolutely no problem. Just wanted to check for updates. No hurry at all, just take your time :-)

@corrados
Copy link
Owner

@thijstriemstra You approved this merge request. Does that mean that you have tested this code and are happy with the results?

@corrados
Copy link
Owner

corrados commented Jun 7, 2024

I'll integrate the code now. Thanks for the contribution.

@corrados corrados changed the base branch from main to integrate_pull_request_137 June 7, 2024 13:46
@corrados corrados merged commit 17c63ad into corrados:integrate_pull_request_137 Jun 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants