search

adafruit / Adafruit_CircuitPython_24LC32

CircuitPython Driver for Adafruit 24LC32 I2C EEPROM Breakout 32Kbit / 4 KB
MIT License
6 stars 6 forks source link

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

Closed tekktrik closed 2 years ago

tekktrik commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

tekktrik commented 2 years ago

Sounds like a plan!