Skip to content

Add support for EEPROM #184

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

Closed
wants to merge 24 commits into from
Closed

Add support for EEPROM #184

wants to merge 24 commits into from

Conversation

jgfoster
Copy link
Member

@jgfoster jgfoster commented Oct 16, 2020

Fixes #166. Also add testing to demonstrate use.

@ianfixes
Copy link
Collaborator

What information should I put into REFERENCE.md for this? It looks like it mocks the functionality, but does not interact with GODMODE in a way that would let you verify that the functions had been called a certain way. (which does not block this PR)

@jgfoster
Copy link
Member Author

Actually it is fair for you to say that we need to update the documentation! How that looks is open for discussion and advice. From the beginning (only a few weeks ago) I've been unsure on how to fit in new mocks. Since the EEPROM functionality doesn't involve pins, I don't see the direct tie-in to GODMODE. As to verifying "that the functions had been called in a certain way," I'm not sure. In this case, the API is pretty much read/write. If there data is there, then then I assume it was called correctly. What more can be tested? (Not intended as pushing back, just trying to figure it out myself.)

Shall I put in a PR with a paragraph describing how to use EEPROM?

@ianfixes
Copy link
Collaborator

Apologies for the delay. It looks like the unit tests are all there, so all that I think remains is a short paragraph in REFERENCE.md similar to the section on SPI -- just saying that it acts like a basic read/write buffer and there is no GODMODE support at this time. (That sort of thing can always be added later -- we can open an issue to track it if/when that functionality is desired).

Add documentation for EEPROM
@ianfixes
Copy link
Collaborator

I'll smooth over the merge conflicts here in a bit

Comment on lines 9 to 12
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
License as published by the Free Software Foundation; either
version 2.1 of the License, or (at your option) any later version.
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably reconsider adding this directly since its license is inconsistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really out of my depth with regard to the terms of all the different licenses, and really can't offer advice one way or another. I have no idea whether your own rewrite counts as some "derivative" in the way the licenses are written.

I think as long as we're doing our best and intend to respond to any legal complaints -- fixing the license as appropriate -- (which I do intend to do) then I'm not sure what else it would take to move forward. In any case, if we keep the file let's keep the license and it can all get sorted out later.

Copy link
Member Author

Choose a reason for hiding this comment

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

As owner of the project, you certainly have the last word on this. But (did you sense a "but" coming?), the difference between the GNU license and your Apache license is night and day (with slight exaggeration, think Catholics and Protestants in 16th, 17th, and 18th century Europe, or Antifa vs. Proud Boys today). While I can't promise that my replacement would withstand a legal challenge, I'm pretty confident that it would be better to leave out the GNU reference. Other than the file name and the published API, I made an effort to write the code from scratch and think that the risk is extremely small. By leaving in the GNU license, you risk attaching GNU to my code, and then you would need another rewrite.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw your rewrite after adding my comment, and I think your rewrite satisfies

@ianfixes ianfixes added this to the OpenAcidification milestone Nov 10, 2020
@ianfixes
Copy link
Collaborator

I've squashed this into #183

@ianfixes ianfixes closed this Nov 11, 2020
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.

Support for EEPROM
2 participants