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

adding simple bit and bits for SPI devices #35

Closed
wants to merge 3 commits into from

Conversation

maholli
Copy link

@maholli maholli commented Feb 8, 2020

I quickly ported over the I2C bit and bits functionality to SPI devices.

Confirmed working on a handful of SPI devices that expect register interaction with the following format: byte1:starting register address byte2:# of registers minus 1. Note this is different than I2C where you don't need byte2 and can just clock in/out data.

A little high-level reworking of the library could likely integrate I2C and SPI together (if someone was motivated).

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for this! I'm a little worried that this will increase the frozen size of the library so that existing boards won't fit anymore. Can one of the @adafruit/circuitpythonlibrarians try building circuitpython on CPX with this new version of the library?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Apr 5, 2020

@tannewt I'm working on giving this a try. I put this modified Register library inside of the frozen directory. Then ran make BOARD=circuitplayground_express however I notice in the output that it did not print anything about freezing this library:

FREEZE ../../frozen/Adafruit_CircuitPython_BusDevice 
../../frozen/Adafruit_CircuitPython_CircuitPlayground 
../../frozen/Adafruit_CircuitPython_HID 
../../frozen/Adafruit_CircuitPython_LIS3DH 
../../frozen/Adafruit_CircuitPython_NeoPixel 
../../frozen/Adafruit_CircuitPython_Thermistor

Looking at the list here: https://github.com/adafruit/circuitpython/blob/e6f11947cb053fc4a9da844a3689c6af5881590d/ports/atmel-samd/boards/circuitplayground_express/mpconfigboard.mk#L22 it seems that Adafruit_CircuitPython_Register is not one of the modules that gets frozen into the CPX build.. Am I understanding that correctly?

@dhalbert
Copy link
Contributor

dhalbert commented Apr 5, 2020

Adafruit_CircuitPython_Register is not one of the modules that gets frozen into the CPX build

That is correct, and it's probably not going to fit, unfortunately.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Apr 5, 2020

Thank you @dhalbert Do you know if there is some sort of testing related to this that is still needed for this PR?

@dhalbert
Copy link
Contributor

dhalbert commented Apr 5, 2020

The Register library is not used by any of the current CPX frozen libraries, so it doesn't need to be frozen, in, and isn't currently. So this improvement can stand on its own.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Apr 5, 2020

This is passing pylint now. I can try testing it, but I'm not sure which kinds of devices expect this type of interaction. Does anyone know of an example of a device that would need this? @maholli are you able to share which devices you used with it?

@maholli
Copy link
Author

maholli commented Apr 5, 2020

@FoamyGuy when I was testing I confirmed spi_bits worked with the TI ADS124S08 24-bit ADC and Bosh BMX160 IMU.

@siddacious
Copy link
Contributor

@FoamyGuy This function in the MAX31856 library reads a bunch of individual bits from a byte of interrupt statuses. It should be fairly straightforward to adapt. We have other libraries that mix Register with manually reading/writing registers, so you should be able to convert just this method to start:

https://github.com/adafruit/Adafruit_CircuitPython_MAX31856/blob/master/adafruit_max31856.py#L225

LMK if you want to take that on and I can provide pointers as need be.

Thanks for this @maholli !

@tannewt
Copy link
Member

tannewt commented Apr 6, 2020

Ok, no concerns here. Will let @FoamyGuy approve and merge.

@tannewt tannewt requested a review from FoamyGuy April 6, 2020 20:34
@FoamyGuy
Copy link
Contributor

FoamyGuy commented Apr 7, 2020

@siddacious I took a try at converting the function you linked to use some of the new classes from this change. I'm not certain if I'm understanding it fully but here is what I came up with:

@property
def fault(self):
    """A dictionary with the status of each fault type where the key is the fault type and the
    value is a bool if the fault is currently active
    ===================   =================================
    Key                   Fault type
    ===================   =================================
    "cj_range"            Cold junction range fault
    "tc_range"            Thermocouple range fault
    "cj_high"             Cold junction high threshold fault
    "cj_low"              Cold junction low threshold fault
    "tc_high"             Thermocouple high threshold fault
    "tc_low"              Thermocouple low threshold fault
    "voltage"             Over/under voltage fault
    "open_tc"             Thermocouple open circuit fault
    ===================   =================================
    """

    return {
        "cj_range":  ROBit(_MAX31856_SR_REG, 7),
        "tc_range": ROBit(_MAX31856_SR_REG, 6),
        "cj_high": ROBit(_MAX31856_SR_REG, 5),
        "cj_low": ROBit(_MAX31856_SR_REG, 4),
        "tc_high": ROBit(_MAX31856_SR_REG, 3),
        "tc_low": ROBit(_MAX31856_SR_REG, 2),
        "voltage": ROBit(_MAX31856_SR_REG, 1),
        "open_tc": ROBit(_MAX31856_SR_REG, 0),
    }

Is this on the right track?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Apr 7, 2020

Would it be possible to test these changes with a BME680 or a PN532? Both of them can use SPI but I don't think they require any special interaction.

If I am understanding correctly I could use the new classes ROBit RWBit etc... to access data within these devices even though it's not required does that sound right?

@FoamyGuy
Copy link
Contributor

@siddacious I think I am in need of a point in the right direction. I was able to get a MAX31856 but I don't think I understand how to use the ROBit class correctly inside of the fault function. I tried code like my post above but now the fault property looks like this:

>>> thermocouple.fault
{'voltage': <ROBit object at 20001fe0>, 'tc_low': <ROBit object at 20001f90>, 
'tc_range': <ROBit object at 20001e90>, 'tc_high': <ROBit object at 20001f40>, 
'cj_range': <ROBit object at 20001e60>, 'cj_high': <ROBit object at 20001ea0>, 
'open_tc': <ROBit object at 20002030>, 'cj_low': <ROBit object at 20001ef0>}

I'm not sure how to properly access the value of these bits once they are created.

@siddacious
Copy link
Contributor

@FoamyGuy close! The ROBits need to go in the body of the class as a peer to the methods. This will add a property-like entity to the class that you can access.

You'll want to do something like:

class MAX31856:
    _cj_range = ROBit(_MAX31856_SR_REG, 7)
    _tc_range = ROBit(_MAX31856_SR_REG, 6)
    # ...more ROBits, __init__, other methods, etc.

    @property
    def fault(self):
        return {
            "cj_range": self._cj_range,
            "tc_range": self._tc_range
            #...other entries
        }

Here's an example of a definition:
https://github.com/adafruit/Adafruit_CircuitPython_HTS221/blob/master/adafruit_hts221.py#L144

and usage:
https://github.com/adafruit/Adafruit_CircuitPython_HTS221/blob/master/adafruit_hts221.py#L247

Sorry for the delayed response! Feel free to @ me on discord if you have any more q's
Thanks for your help :)

@FoamyGuy
Copy link
Contributor

I did some testing on this using some tweaks to the MAX31856 library. Using the debug_spi.py we can see the differences in behavior of these RWBit and RWBits as compared to the unmodified library code.

Unmodified MAX library:

self._write_u8(_MAX31856_CR0_REG, _MAX31856_CR0_OCFAULT0)
# output:
# spi.write: ['0x80', '0x10', '0x0', '0x0']

MAX library modified to use RWBits:

_cr0_reg = RWBits(8, _MAX31856_CR0_REG, 0)

# ... further down inside __init__():

self._cr0_reg = 0x10
# output:
# spi.write: ['0x0', '0x0']
# spi.readinto: ['0x0', '0x3']
# spi.write: ['0x0', '0x10']

Lastly with the MAX library modified to use RWBit as a simple test:

_ocfault0 = RWBit(_MAX31856_CR0_REG, 4)

# ... further down inside __init__():
print(self._ocfault0);
self._ocfault0 = True
print(self._ocfault0);

# output:
# spi.write: ['0x0', '0x0']
# spi.readinto: ['0x0', '0x3']
# False
# spi.write: ['0x0', '0x3']
# spi.readinto: ['0x0', '0x3']
# spi.write: ['0x0', '0x13']
# spi.write: ['0x0', '0x13']
# spi.readinto: ['0x0', '0x3']
# False

:param bool lsb_first: Is the first byte we read from spi the LSB? Defaults to true

"""
def __init__(self, register_address, bit, register_width=1, lsb_first=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider where it belongs, but we're going to need to configure SPI registers with something similar to the SPI type enum in Adafruit_BusIO, the Arduino equivalent of Adafruit_CircuitPython_Register:
https://github.com/adafruit/Adafruit_BusIO/blob/master/Adafruit_BusIO_Register.h#L8

At least the RW bit polarity will need to be set and supported since this is to my knowledge the only way SPI has of designating Read vs Write. Additionally the address increment bit should be supported as well as it's nearly as common.

I think that behavior and configuration is best placed in CircuitPython's busio.SPI as the equivalent behavior for I2C is handled there as well and it probably doesn't need to be specified per-register.
@tannewt @dhalbert @jepler, your input would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

@siddacious Could you elaborate and give an example? I'm not sure of the context here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@siddacious Can you please follow up on this PR? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@kattni I'm going to finally get back to this and address @tannewt's request for more info!

Copy link
Contributor

Choose a reason for hiding this comment

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

@tannewt to elaborate:
SPI uses a bit within the first byte sent to designate if the following bytes will be read or written by the host. As far as I know, it's up to the manufacturer to decide if a high level designates a read or a write. Here is the relevant section of the datasheet for the bosch IMU mentioned by Dr. M:
image

I'm suggesting the polarity of this should be able to be set in the constructor of busio.SPI, with a default to rw_high_write=True as it is the most common.
edit: see below

Additionally, SPI devices often use a bit in the address byte to tell the sensor to automatically increment the register address for successive reads, which allows you to read contiguous registers without specifying the address of the next register each time. Getting this right is particularly important because if it isn't set correctly, you can still get data back that seems valid but that will just be repeats of the data from the register with the specified address.

Here is an example from the bosch imu datasheet that implies that such a bit isn't needed:
image

And one from the LIS3DH that explains that it is:
image

The proper setting of these bits is necessary for the proper functioning of the sensor library, and since the setting will always be the same for a given sensor, ideally it would be able to be set once. I was originally suggesting that this should be set in the SPI constructor, however since an SPI bus can be shared, it has the potential to cause issues if some of the sensors on the bus have different settings.

That said, the clock polarity and phase can be different between sensors and it is set in the busio.SPI object. It's not clear to me at this point where the ideal place to set this would be. Perhaps something like a Register factory object that is then used to create the individual RWBits, etc.

I've had this ball in my court for a while now and I'd like to see SPI support in Register, so any input that will help us make a choice and move forward would be appreciated.

PS: here's a graphic that explains the SPI clock polarity and phase settings for those who want more info:

from: https://en.wikipedia.org/wiki/Serial_Peripheral_Interface#Clock_polarity_and_phase
image

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

Choose a reason for hiding this comment

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

@siddacious I think the simplest thing is to have it as part of the register field's constructor. Then it can change the top bit or two as needed. In the longer term it'd be interesting to unify the bus API so we can have a device with registers accessible over i2c or spi.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

Copy link
Contributor

Choose a reason for hiding this comment

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

@tannewt to keep this moving, should I look into adding the RW/multi-byte bit behavior?

Copy link
Member

Choose a reason for hiding this comment

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

@siddacious If I remember right, you'll want two kwargs. One which is the bit position to indicate read vs write and then one bit position to indicate no address increment.

@jposada202020
Copy link

@tannewt :)
This was an interesting reading. What would be the future of this PR? taking all aspects in consideration. what could be done to keep this going?

My only comment:

Board with reduced flash memory:
register + bus device + sensor library + user code = it is very constraint.

@evaherrada evaherrada changed the base branch from master to main June 2, 2021 15:01
@tannewt
Copy link
Member

tannewt commented Jun 2, 2021

@jposada202020 I think the best thing would be to make a new library specifically for register over SPI.

@FoamyGuy
Copy link
Contributor

Closing this for now.

@maholli if you're interest in spinning this off into it's own library/repo for registor over SPI we could add it to the community bundle

@FoamyGuy FoamyGuy closed this Dec 21, 2021
@maholli
Copy link
Author

maholli commented Dec 21, 2021

Closing this for now.

@maholli if you're interest in spinning this off into it's own library/repo for registor over SPI we could add it to the community bundle

Sounds good. If someone starts this, please ping me.

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

7 participants