Initial FRAM I2C #1

sommersoft · 2018-10-02T23:39:58Z

Here it be. Fixes Part 1 of CircuitPython Issue #665

I assume RTD sub-project will need to be created, but that can wait until I finish the SPI version (separate PR).

Also, I'm going to put in an issue to update the FRAM Learn Guide with CircuitPython. I'm happy to do it, just need to be added to the guide.

sommersoft · 2018-10-02T23:40:55Z

Annnddd, Travis needs to be setup on this repo.

Also...needs librarians added to admins. (will this guy ever stop talking? 😆)

kattni · 2018-10-03T00:40:17Z

Added Librarians to the repo. Activated the repo on Travis, however it's not seeing the PR. I'll look more into it tomorrow.

caternuson

General approach looks good. See comments about changes to interface.

caternuson · 2018-10-14T01:33:59Z

adafruit_fram.py

+            status = self._wp_pin.value
+        else:
+            status = self._wp
+        return status


Could replace this entire function with:

return self._wp if self._wp_pin is None else self._wp_pin.value

caternuson · 2018-10-14T01:38:32Z

adafruit_fram.py

+        if not self._wp_pin is None:
+            self._wp_pin.value = value
+
+    def write_protect_pin(self, wp_pin, write_protect=False):


Are there scenarios where this would need to be set or changed after creating the object? If not, remove this and require passing it in through __init__.

I originally wrote it where the pin was only passed through during __init__. I added this later, as a "nicety" for the user. No specific scenario in mind... I am fine with removing it.

caternuson · 2018-10-14T01:39:44Z

adafruit_fram.py

+        """
+        if not wp_pin is None:
+            import digitalio
+            self._wp_pin = digitalio.DigitalInOut(wp_pin)


Same comment here about pin object vs. designation.

caternuson · 2018-10-14T01:50:16Z

adafruit_fram.py

+            read_buffer[i] = self._read_byte(register + i)[0]
+        return read_buffer
+
+    def write_single(self, register, data):


Use __setitem__ for this to allow for fram[address] = value.

fram[0x23] = 0x42

caternuson · 2018-10-14T01:54:14Z

adafruit_fram.py

+                                                        self._max_size))
+        self._write_register(register, data)
+
+    def write_sequence(self, start_register, data, wraparound=False):


This can also be done through __setitem__.

fram[start_address] = data

caternuson · 2018-10-14T01:58:10Z

adafruit_fram.py

+        prod_id = (((read_buf[1] & 0x0F) << 8) + read_buf[2])
+        if (manf_id != _I2C_MANF_ID) and (prod_id != _I2C_PROD_ID):
+            raise OSError("FRAM I2C device not found.")
+        i2c_bus.unlock()


I2C bus creation should be done by the calling program and passed in. See how other I2C device drivers are doing this.
The MID/PID check to see if the device is connected and found is good though. Just change to doing that through the I2CDevice.

I can change to, and agree with, having the I2C bus passed in. However, I'll still need to use the raw bus to get the MID/PID due to the following:

The Device ID (MID/PID) is at specific address, which is different from the I2C address space that is passed in.

Reading the Device ID requires a certain order of writes/reads, which I2CDevice would not allow. When I2CDevice is initialized, it writes an empty buffer to the stored address, which would (did) interfere with the specified steps.

caternuson · 2018-10-14T02:04:45Z

examples/fram_i2c_simpletest.py

+## Read that byte to ensure a proper write.
+## Note: 'read()' returns a bytearray
+
+print(fram.read(0)[1])


Off by one. fram.read(0)[0]
But this will become

print(fram[0])

caternuson · 2018-10-14T02:12:04Z

Just documenting my testing of the basic functionality of current version.

Adafruit CircuitPython 3.0.3 on 2018-10-10; Adafruit ItsyBitsy M4 Express with samd51g19
>>> import board
>>> import adafruit_fram
>>> fram = adafruit_fram.FRAM_I2C(board.SCL, board.SDA)
>>> fram.max_size
32767
>>> fram.write_single(0, 1)
>>> print(fram.read(0)[0])
1
>>> fram.write_protected
False
>>> fram.write_protected = True
>>> fram.write_protected
True
>>> fram.write_single(1, 23)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_fram.py", line 165, in write_single
RuntimeError: FRAM currently write protected.
>>>

caternuson · 2018-10-14T15:52:34Z

adafruit_fram.py

+                self._wp_pin.deinit()
+            self._wp_pin = None
+
+    def read(self, register, length=1):


Use__getitem__ for this.

value = fram[0] values = fram[23:42]

sommersoft · 2018-10-15T05:11:34Z

Example of slice handling. Its rudimentary...doesn't support negative values, but aligns with CPython range assignment other than that.

>>> fram[::1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./circuitpython_libs/Adafruit_CircuitPython_FRAM/adafruit_fram.py", line 128, in __getitem__
MemoryError: memory allocation failed, allocating 4096 bytes
>>> fram[:100]
bytearray(b'\x00\x01\x1elohello\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\
x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\
x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\
x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\
x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')
>>> fram[1:10:10]
bytearray(b'\x01')
>>> fram[1]
bytearray(b'\x01')
>>> fram[1:10]
bytearray(b'\x01\x1elohello')
>>> fram[:32760]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./circuitpython_libs/Adafruit_CircuitPython_FRAM/adafruit_fram.py", line 128, in __getitem__
MemoryError: memory allocation failed, allocating 4096 bytes
>>> fram[32760:]
bytearray(b'\x00\x00\x00\x00\x00\x00\x00')
>>> fram[0:1:1] = [0,1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./circuitpython_libs/Adafruit_CircuitPython_FRAM/adafruit_fram.py", line 160, in __setitem__
ValueError: Slice steps are not allowed during write operations.
>>> fram[0:5] = [0,1,2,3,4]
>>> fram[0:5]
bytearray(b'\x00\x01\x02\x03\x04')

caternuson

New item access looks good. Found a few more tweaks.

caternuson · 2018-10-15T17:30:08Z

adafruit_fram.py

+            if key > self._max_size:
+                raise ValueError("Register '{0}' greater than maximum FRAM size."
+                                 " ({1})".format(key, self._max_size))
+            return self._read_byte(key)


Maybe have this be:

return self._read_byte(key)[0]

so you just get back the value instead of a single element bytearray with the value.

Meant to comment on this last night. With the change to __getitem__, I wanted to make the return value standard. I'm not completely opposed to sending back the value. Just felt that reduced API confusion was more beneficial.

caternuson · 2018-10-15T17:35:12Z

adafruit_fram.py

+        if isinstance(key, int):
+            if not isinstance(value, int):
+                raise ValueError("Data must be an integer.")
+            if key.start > self._max_size:


Looks like slice code? Trying to use .start on an int type.

hmm...swear I fixed that on the fix the fixes commit.

caternuson · 2018-10-15T17:35:35Z

adafruit_fram.py

-        for i in range(length):
-            read_buffer[i] = self._read_byte(register + i)[0]
-        return read_buffer
+            self._write_register(key.start, value)


Same here - trying to use .start on an int type.

caternuson · 2018-10-16T23:43:18Z

adafruit_fram.py

+    #pylint: disable=too-many-arguments
+    def __init__(self, i2c_bus, address=0x50, write_protect=False,
+                 wp_pin=None):
+        from adafruit_bus_device.i2c_device import I2CDevice as i2cdev


Do you have this here so it only gets imported when the I2C class is used?

OK. We may end up refactoring the various classes into other files. It's been done in other libraries, but I don't think we're being too consistent about it. This will become more important as this grows with the addition of the code for the SPI class.

tannewt

A couple comments but looks good otherwise!

tannewt · 2018-10-18T17:49:30Z

adafruit_fram.py

+
+            read_buffer = bytearray(len(registers))
+            for i, register in enumerate(registers):
+                read_buffer[i] = self._read_byte(register)[0]


Is it possible to read more than one byte at a time? That will be much faster.

At this point, I don't remember why I chose to read one byte at a time. May have been an attempt to minimize memory use.

Just ran some comparisons; memory allocation is tripped at the same length of reads both ways. I'll switch it. (it is much faster)

tannewt · 2018-10-18T17:50:21Z

adafruit_fram.py

+            self._wp_pin = wp_pin
+
+    @property
+    def max_size(self):


Should this be the standard __len__ instead? That way you can use range on it in a loop.

I have no issue with changing to __len__, if that is more Pythonic. Since __getitem__ and __setitem__ are already documented as special members, I can just add this too.

sommersoft · 2018-10-19T22:39:33Z

Don't merge yet...

I need to fix __setitem__; slicing touchup and handling len(value) > slice[start:stop]

>>> fram[0:4] = bytearray(b'length>greater_than_slice')
>>> fram[0:4]
bytearray(b'leng')
>>> fram[0:15]
bytearray(b'length>greater_')

tannewt · 2018-10-22T22:53:50Z

@sommersoft Ping me when you need another review.

sommersoft · 2018-10-23T01:53:41Z

@tannewt ready for another look.

note: dropped slicing altogether for __setitem__, and removed slice stepping for __getitem__.

kattni · 2018-10-29T21:30:42Z

@tannewt ping.

tannewt

Looks great! Thanks!

sommersoft added 7 commits September 30, 2018 22:46

initial I2C library

1b63072

initial I2C library, part duex

a2536df

update MAX SPI size...to trigger Travis build.

d9673bd

pylint fixes

c80ec10

Sphinx fix; attempt adafruit#1

b97d359

start param with two dots...in the proper vertical orientation. 🤦

6e12b3b

Sphinx fix: product links

1251ebb

Docstring formatting...to kick Travis.

5546608

sommersoft requested a review from a team October 3, 2018 02:29

corrected max values

3deda9f

kattni mentioned this pull request Oct 6, 2018

CircuitPython Libraries! adafruit/circuitpython#1246

Open

caternuson requested changes Oct 14, 2018

View reviewed changes

sommersoft added 2 commits October 14, 2018 23:48

change request fixes; getter/setter, external objects.

0ebf851

fix to the fixes; slice handling.

fbd6a0e

pylint fixes

4c5a58b

caternuson requested changes Oct 15, 2018

View reviewed changes

sommersoft added 2 commits October 15, 2018 21:24

Device ID now read with I2CDevice; updated docstring for getter/setter

8166bbf

fix fail; this fixes the fail. maybe.

2544b4f

caternuson reviewed Oct 17, 2018

View reviewed changes

caternuson approved these changes Oct 17, 2018

View reviewed changes

tannewt reviewed Oct 18, 2018

View reviewed changes

sommersoft added 2 commits October 19, 2018 16:21

change read() to read the whole request at once; not one byte at a time

5666a1b

change 'max_size()' to '__len__'

8180928

clean up slicing; PEP8isms

f58f172

sommersoft added 2 commits October 22, 2018 20:06

docstring update; Travis re-sync kick.

59a826f

pylint fix

3005592

tannewt approved these changes Nov 4, 2018

View reviewed changes

tannewt merged commit 8c66050 into adafruit:master Nov 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial FRAM I2C #1

Initial FRAM I2C #1

sommersoft commented Oct 2, 2018

sommersoft commented Oct 2, 2018 •

edited

Loading

kattni commented Oct 3, 2018

caternuson left a comment

caternuson Oct 14, 2018

caternuson Oct 14, 2018

sommersoft Oct 14, 2018

caternuson Oct 14, 2018

caternuson Oct 14, 2018

caternuson Oct 14, 2018

caternuson Oct 14, 2018

sommersoft Oct 14, 2018 •

edited

Loading

caternuson Oct 14, 2018

caternuson commented Oct 14, 2018

caternuson Oct 14, 2018

sommersoft commented Oct 15, 2018

caternuson left a comment

caternuson Oct 15, 2018

sommersoft Oct 16, 2018

caternuson Oct 15, 2018

sommersoft Oct 15, 2018

caternuson Oct 15, 2018

caternuson Oct 16, 2018

sommersoft Oct 17, 2018

caternuson Oct 17, 2018

tannewt left a comment

tannewt Oct 18, 2018

sommersoft Oct 19, 2018

tannewt Oct 18, 2018

sommersoft Oct 19, 2018

sommersoft commented Oct 19, 2018

tannewt commented Oct 22, 2018

sommersoft commented Oct 23, 2018

kattni commented Oct 29, 2018

tannewt left a comment

Initial FRAM I2C #1

Initial FRAM I2C #1

Conversation

sommersoft commented Oct 2, 2018

sommersoft commented Oct 2, 2018 • edited Loading

kattni commented Oct 3, 2018

caternuson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sommersoft Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caternuson commented Oct 14, 2018

Choose a reason for hiding this comment

sommersoft commented Oct 15, 2018

caternuson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sommersoft commented Oct 19, 2018

tannewt commented Oct 22, 2018

sommersoft commented Oct 23, 2018

kattni commented Oct 29, 2018

tannewt left a comment

Choose a reason for hiding this comment

sommersoft commented Oct 2, 2018 •

edited

Loading

sommersoft Oct 14, 2018 •

edited

Loading