Skip to content

[2.0.x] PersistentStore update followup#11549

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
p3p:pr_bf2_duefix
Aug 14, 2018
Merged

[2.0.x] PersistentStore update followup#11549
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
p3p:pr_bf2_duefix

Conversation

@p3p
Copy link
Member

@p3p p3p commented Aug 14, 2018

Remove duplication of renamed files caused by merge and fix LPC1768 and due builds with flash emulated EEPROM, also MarlinConfig.h is required for serial and the emulated EEPROM functions.

Added a EEPROM_SETTINGS test to LPC1768 and Due tests, the tests probably need expanded on now the platforms are ran in parallel and it doesn't take 30 mins.

#11543

@marcio-ao
Copy link
Contributor

@p2p: I can confirm that this patch allows me to compile, once I fix a likely unrelated error:

In "HAL/HAL_DUE/Servo_Due.cpp", #include "../servo.h needs to be #include "../shared/servo.h"

@p3p
Copy link
Member Author

p3p commented Aug 14, 2018

ah that's from the shared folder PR, I fixed a few others to do with that so may as well fix that one too.

Remove duplication of renamed files and fix due builds with flash emulated, MarlinConfig.h is required for serial and other functions.
Although platforms.h is shared it makes sense to be in the HAL folder as it controls header redirects and needs updated when a new platform folder is added
@thinkyhead
Copy link
Member

Anything else to add before we merge this?

@p3p
Copy link
Member Author

p3p commented Aug 14, 2018

I don't think so.., Not sure if you want to just use MarlinConfig.h or are OK with using MarlinConfigPre.h then only if the feature is enabled pulling in the rest of the headers,

There are things that should probably be done, renaming E2END and changing the spi and i2c eeprom to use PersistantStore but that's for another PR.

@thinkyhead
Copy link
Member

MarlinConfigPre.h is preferred at the top of source files where only the configuration values are needed (to test for enabled config options). MarlinConfig.h should be used when MarlinConfigPre.h doesn't provide enough root headers to allow the code to compile.

@thinkyhead
Copy link
Member

…renaming E2END…

We use that name because it's what the Arduino platform headers define. We can't entirely eliminate its use for the platforms that use it to determine the EEPROM size.

@thinkyhead thinkyhead merged commit 5573ef6 into MarlinFirmware:bugfix-2.0.x Aug 14, 2018
@p3p
Copy link
Member Author

p3p commented Aug 14, 2018

We can outside of the platforms though, the only usage in Marlin core is now in the pins files where it is defined to specify the size of i2c/spi attached EEPROMs.

@thinkyhead
Copy link
Member

thinkyhead commented Aug 14, 2018

Sounds like we should roll the i2c-based methods into the PersistentStore implementations and just use the capacity, as established for the other storage types. i.e., persistent_store_i2c.cpp

@p3p
Copy link
Member Author

p3p commented Aug 14, 2018

Indeed that was the plan, was one of the reasons I wrote the PersistentStore api to begin with along with unified api for platforms.

There will probably 3-4 shared implementations rather than platform specific ones
persistent_store_eeprom (using the avr eeprom interface)
persistent_store_i2c (wire interface)
persistent_store_spi (marlins spi interface)
persistent_store_sdcard (Marlins sdcard interface, STMF1 platform has an example of this)

@p3p p3p deleted the pr_bf2_duefix branch August 14, 2018 23:41
@fiveangle
Copy link

In "HAL/HAL_DUE/Servo_Due.cpp", #include "../servo.h needs to be #include "../shared/servo.h"

It must have been 4 times I'd re-done this exact change in the Servo_Due.cpp so not sure where the reverts kept coming from in my chain.

I've burned an old Fortran 77 reference book in effigy to the git gods and will monitor to ensure no similar shenanigans..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants