Skip to content

Add Mock Support for EEPROM #174

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

Merged
merged 8 commits into from
Oct 9, 2020
Merged

Add Mock Support for EEPROM #174

merged 8 commits into from
Oct 9, 2020

Conversation

jgfoster
Copy link
Member

@jgfoster jgfoster commented Oct 6, 2020

Previously we had a hard-coded size that assumed all boards has EEPROM. This defines it only for select boards and I'm adding Mega to that list.

*
* See https://github.com/Arduino-CI/arduino_ci/issues/166#issuecomment-703904394.
*/
#define EEPROM_SIZE (4096)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be more appropriate to conditionally set this variable based on the board being used, which can be done in the default configuration here:
https://github.com/Arduino-CI/arduino_ci/blob/master/misc/default.yml#L102

Copy link
Member Author

Choose a reason for hiding this comment

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

I can certainly try that. By way of background, where did those files come from? Why does EEPROM_SIZE exist in over 40 of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I copied them from the Arduino project itself, arduino-1.8.5/hardware/tools/avr/avr/include/avr to get things to compile... some hand-edits were made. I have very little information on how/why Arduino structures these files.

Copy link
Contributor

Choose a reason for hiding this comment

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

where did those files come from?

It's avr-libc:
https://www.nongnu.org/avr-libc/
Nothing Arduino-specific. This is just the standard open source AVR toolchain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does EEPROM_SIZE exist in over 40 of them?

Because there are a lot of different AVR microcontrollers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd really love to find a way to avoid packaging all of these files in this repo but I'm at a loss for how to do so (while enabling a non-AVR compiler to supply all the #define statements some alternate way).

Copy link
Member Author

@jgfoster jgfoster Oct 6, 2020

Choose a reason for hiding this comment

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

Because there are a lot of different AVR microcontrollers.

But why isn't it defined for Mega2560? That microcontroller has EPROM in it. I was hoping that the macro would be a reliable way to determine whether the feature is present. Instead it seems that it has __EEPROM_REG_LOCATIONS__ but not the size.

Copy link
Member Author

Choose a reason for hiding this comment

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

The files are mostly out of the way and don't bother me much; I just find myself expecting more of them than I should given my limited knowledge!

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I see now. The macros are artifacts of the implementation, not a way of giving information to clients of the library, and different implementations will have different macros. That makes sense and is reasonable.

Further investigation shows that E2END exists in 230 files and many times is (0x0). Perhaps it is a more reliable macro to use.

@ianfixes
Copy link
Collaborator

ianfixes commented Oct 6, 2020

@matthijskooijman will it make #144 difficult if I merge this one first?

@jgfoster
Copy link
Member Author

jgfoster commented Oct 6, 2020

@ianfixes, I think #144 is a PR for master, while this is for tdd.

@ianfixes
Copy link
Collaborator

ianfixes commented Oct 7, 2020

Understood, let me know when you have all your desired commits in and I'll squash & merge.

@jgfoster
Copy link
Member Author

jgfoster commented Oct 7, 2020

I think this is ready to be merged into tdd so that substantive tests can be contributed by others. (Of course, I may find more I want to change as well!)

@jgfoster jgfoster changed the title Add EEPROM_SIZE to Mega Add Mock Support for EEPROM Oct 8, 2020
@jgfoster
Copy link
Member Author

jgfoster commented Oct 8, 2020

@ianfixes, This is ready to be merged into ttd so others can contribute additional tests.

@ianfixes ianfixes merged commit 3502211 into Arduino-CI:tdd Oct 9, 2020
@jgfoster jgfoster deleted the eeprom branch October 14, 2020 15:31
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