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

Update ValueError raised when storing non-integer in index #4

Merged
merged 3 commits into from Dec 20, 2021
Merged

Update ValueError raised when storing non-integer in index #4

merged 3 commits into from Dec 20, 2021

Conversation

tekktrik
Copy link
Member

This fixes the issue mentioned the equivalent issue mentioned here in the FRAM library:

Also, updates example to use correct indexes as fixed with PR #1

@dglaude
Copy link

dglaude commented Dec 20, 2021

Thank you, I did not check this pull request yet, but:

(1) This week-end I was hit by the "ValueError" message and a bit confused by the wording. I was actually trying to convert my value that was int already into a byte or single byte byte array. So any hint on the proper usage would have saved me a little bit of time there

(2) I feel the example code should do read and comment out the write, I am always afraid to wear out technology I don't know very well and I don't know the writing live span. But your removal of the while loop is welcomed.

(3) Your last part would fix #2

Maybe part of this pull request you would like to chasse from this file all the reference to FRAM as mentioned in #5 .
I can make a separated PR, but it always confuse me when more than one PR is pending and they could conflict with each other.

@tekktrik
Copy link
Member Author

Hey @dglaude yeah, that error text comes from my somewhat recent PR to the FRAM library this was based on, so I wanted to make sure that any updates pertaining to both get addressed. I tried to only update things that were in the scope of that (and also making sure the test code ran) but don't want to get in @FoamyGuy's way too much because I know this repo is still WIP. :) I'm sure the FRAM references will be changed as the library develops.

@FoamyGuy
Copy link
Collaborator

Commenting out the write in the sounds good to me, much appreciated if either of you want to make a separate PR for that.

We will get the references to "fram" removed before any official releases are made.

Learning that this library works with other EEPROM breakouts makes me think we might actually want to have Adafruit_CircuitPython_EEPROM library instead of the current more specific Adafruit_CircuitPython_24LC32. That would make the name of the library a little more general and we can have some different classes with the needed constants for different EEPROM breakouts that are available, and possibly allow user to pass in max_size as an init argument for use with other EEPROM devices not specifically included but that do happen to use the same communication protocol.

I've made a note for the "in the weeds" section of the meeting today to ask about the naming of this library so we can see what the team thinks and then create new repo or rename as needed. In the meantime I'll go ahead and merge this PR (and commenting write statements, or anything else that we want to do in the interim). If we do move to a new repo I'll copy over the latest version of this one to it so we start from the same spot.

Copy link
Collaborator

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @tekktrik and thank you to @dglaude as well for trying this library out on some other devices.

@FoamyGuy FoamyGuy merged commit f8fc83f into adafruit:main Dec 20, 2021
@tekktrik
Copy link
Member Author

Sounds like a plan!

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.

None yet

3 participants