mrrwa / NmraDcc

NMRA Digital Command Control (DCC) Library
GNU Lesser General Public License v2.1
137 stars 53 forks source link

using library without EEPROM #38

Closed davidzuhn closed 1 year ago

davidzuhn commented 4 years ago

I'm trying to use an Adafruit Metro M0 (ARM Cortex M0 SAMD21blah based), and this device's Arduino library does not provide the EEPROM interface.

I've noodled up a generic CV database interface, and written one implementation that uses EEPROM and another that uses a file within the local filesystem (since I've got more than just N NMRA DCC CVs to store for my persistence needs).

My first plan was to set this up as a required reference argument to the NmraDcc constructor, and then use the generic interfaces within the NmraDcc code. This could probably even be done without a user sketch code change, where the NmraDcc() constructor would create an EEPROM object, and the NmraDccc(NmraCVDB &) constructor would allow the user to provide their own.

However, in putting the pieces together, I find that parts of the NmraDcc module are member functions within the object, and other parts are just standalone functions (which don't have access to the object internals).

Is there a fundamental reason for this separation of code between methods and functions?

Would you be interested in changes that move the "external" functions inside of the NmraDcc class, which would also permit the "externalization" of the EEPROM interface?

Alternately, I could try to rig this up using a separate standalone CV DB class, but that's doing even more to limit this code to only ever supporting a single DCC interface (which I think is already the case). And I think these changes would do more to break all existing code (or more specifically, require additions to existing sketches to add the declaration of the EEPROM (or other) based CV DB object.

railnerd commented 4 years ago

Per the Arduino.cc forum, there is a new "FlashStorage.h" which might be helpful here.

https://forum.arduino.cc/index.php?topic=581475.0

Pretty sure it leverages this Atmel/Microchip support:

http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-42258-ASF-Manual-SAM-D21_AP-Note_AT07627.pdf

davidzuhn commented 4 years ago

The issue is not that I need to find a replacement for EEPROM (I have already done so).

The issue is that EEPROM use is hardcoded into the NmraDcc code. So something has to be done within NmraDcc to use something other than EEPROM.

One way (maybe) to fix this more generally would be to replace EEPROM within NmraDcc with FlashStorage. Another (my preference) is to let the NmraDcc code deal with the subtleties of DCC signal decoding and processing, and then have that code call out to something else to provide the higher level details (like CV value persistence). Then the application author could use any such means that they like to manage those high level functions. For those who want to use EEPROM (and where it's available), one of the high level modules to do that could indeed be a (very!) thin layer atop EEPROM.

I'm perfectly capable of using my own fork of the NmraDcc code to solve this however I choose to. If I have do, I'll do that. But I really would rather see the solutions get pushed upstream; hence my general level questions for the maintainers (pull requests will follow later).

kiwi64ajs commented 4 years ago

Hi Guys,

On 4/06/2020, at 12:14 PM, david d zuhn notifications@github.com wrote:

I'm trying to use an Adafruit Metro M0 (ARM Cortex M0 SAMD21blah based), and this device's Arduino library does not provide the EEPROM interface.

I've noodled up a generic CV database interface, and written one implementation that uses EEPROM and another that uses a file within the local filesystem (since I've got more than just N NMRA DCC CVs to store for my persistence needs).

My first plan was to set this up as a required reference argument to the NmraDcc constructor, and then use the generic interfaces within the NmraDcc code. This could probably even be done without a user sketch code change, where the NmraDcc() constructor would create an EEPROM object, and the NmraDccc(NmraCVDB &) constructor would allow the user to provide their own.

However, in putting the pieces together, I find that parts of the NmraDcc module are member functions within the object, and other parts are just standalone functions (which don't have access to the object internals).

Is there a fundamental reason for this separation of code between methods and functions?

Would you be interested in changes that move the "external" functions inside of the NmraDcc class, which would also permit the "externalization" of the EEPROM interface?

Hmmm… this is probably not explicitly intended behaviour - more likely I overlooked it when it got made into an Arduino Library from the previous WinAVR GCC world…

Have you got your proposed changes in a Git Repo we can view?

Alternately, I could try to rig this up using a separate standalone CV DB class, but that's doing even more to limit this code to only ever supporting a single DCC interface (which I think is already the case). And I think these changes would do more to break all existing code (or more specifically, require additions to existing sketches to add the declaration of the EEPROM (or other) based CV DB object.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mrrwa/NmraDcc/issues/38, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5Y53KTWDL4ZMRVZWKLTJ3RU3ROHANCNFSM4NSEHGDA.

Back when I started this I’m pretty sure it was int eh WinAVR makefile world and almost certainly pre-Arduino and pre-EEPROM library. At that time the notifyCV* weak reference functions were a way to both validate CV call values and even/maybe provide an alternative CV storage mechanism, but its probably never really been proven - not like you’re wanting.

Later on EEPROM became the blessed way to access the AVR’s EEPROM (when NmraDcc only ran on an AVR) so that pretty much welded the need for the EEPROM library to be present.

Now that Franz-Peter’s generic Arduino support has opened up the library’s potential use to a much wider family of chips I guess it would be good to revisit the need for EEPROM as its now a limitation.

It would be preferable to provide a backwards compatibility layer/macros in place so current code will compile as there are hundreds of users with copy/paste/tweaked sketches based on the examples and Geoff Bunza’s articles etc. breaking those will cause a lot of support emails and frustrations.

However provide a way forward with ARM and ESP chips that don’t support the same EEPROM type interface would also extend the library’s future use on other storage mechanisms.

Maybe its time for a NmraDcc2 library, which doesn’t even try to keep backwards compatibility.

Maybe we do some sort of horrible #ifdef mess that does the right thing for either world… Doesn’t sound very tidy but "goes is good” also works for me ;)

HTH

Alex

billswartz7 commented 3 years ago

I have a version where I ported the code to Seeed Studio XIAO. I have implemented a conditional compile so the EEPROM code works for getting and setting CVs. Attached are files. I also made changes to FlashStorage.h so that the read operations would be very efficient. The XIAO only has emulated storage. If can be backwards compatible so only one library is needed.

NmraDcc.h.txt NmraDcc.cpp.txt FlashStorage.h.txt

kiwi64ajs commented 3 years ago

I had thought the various notifyCVxxx call-backs were sufficient to completely override the EEPROM storage mechansim - however I’d never tried to do so myself so perhaps its a wrong assumption.

Perhaps we should have a separate CVStorage class that we can pass into the library Constructor to override the EEPROM behaviour.

Alex

On 4/06/2020, at 12:32 PM, david d zuhn @.***> wrote:

The issue is not that I need to find a replacement for EEPROM (I have already done so).

The issue is that EEPROM use is hardcoded into the NmraDcc code. So something has to be done within NmraDcc to use something other than EEPROM.

One way (maybe) to fix this more generally would be to replace EEPROM within NmraDcc with FlashStorage. Another (my preference) is to let the NmraDcc code deal with the subtleties of DCC signal decoding and processing, and then have that code call out to something else to provide the higher level details (like CV value persistence). Then the application author could use any such means that they like to manage those high level functions. For those who want to use EEPROM (and where it's available), one of the high level modules to do that could indeed be a (very!) thin layer atop EEPROM.

I'm perfectly capable of using my own fork of the NmraDcc code to solve this however I choose to. If I have do, I'll do that. But I really would rather see the solutions get pushed upstream; hence my general level questions for the maintainers (pull requests will follow later).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mrrwa/NmraDcc/issues/38#issuecomment-638530611, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5Y53O2RSWFRULQQFX2CSDRU3TSPANCNFSM4NSEHGDA.

billswartz7 commented 3 years ago

I had thought the various notifyCVxxx call-backs were sufficient to completely override the EEPROM storage mechansim - however I’d never tried to do so myself so perhaps its a wrong assumption. Perhaps we should have a separate CVStorage class that we can pass into the library Constructor to override the EEPROM behaviour. Alex

The problem is the base code includes "EEPROM.h" which doesn't exist on machines with emulated EEPROM. You would need at least a conditional compile. I didn't think to use that class of callbacks and it looks like that would work as well. I thought to keep things as simple as possible. I modified the FlashStorage.h file but that isn't necessary. I did it for convenience. I think it is relatively easy to support many devices that use flash storage for non-volatile EEPROM.

kiwi64ajs commented 1 year ago

I've just pushed an update to the library, version 2.0.14, that will use the FlashStorage_SAMD library to provide emulated EEPROM on the SAMD platform. Give this a try on a Xiao and see how you get on.