Add Raspberry Pi Sense HAT-related drivers #24
Conversation
| try { | ||
| boolean trigger = gpio.getValue(); | ||
| if (trigger) { | ||
| I2cDevice i2c = mPioService.openI2cDevice(mI2cBus, mI2cAddress); |
There was a problem hiding this comment.
maybe you could keep the I2CDevice has a member and open it in connect?
There was a problem hiding this comment.
Sounds better to make the I2cDevice as an instance variable, I will fix it soon. However, it may not be a good idea to put the openI2cDevice in connect, as I previously found that this may occupy the I2C bus and block other devices using at the same time.
There was a problem hiding this comment.
@MacroYau can you report a bug about this template, it doesn't sound like openI2cDevice should be blocking communication on other device.
There was a problem hiding this comment.
Sure, I will verify again if this is an issue on my side or platform-wise (I tested this several weeks ago), and report it if appropriate. Thanks @proppy!
There was a problem hiding this comment.
@proppy I suppose that this is indeed not a bug but the way I2C works (one active "opened" device at a time). Since all peripherals on Sense HAT shares the same I2C bus, the joystick works like this:
- The ATtiny onboard receives joystick input.
- ATtiny drives GPIO
BCM23high as an interrupt signal for joystick event. - Raspberry Pi monitors
BCM23, and initiates I2C connection with ATtiny at0x46. - Raspberry Pi reads the joystick key state via the I2C register at
0xF2. - Closes I2C connection afterwards to free up the bus for other peripherals.
So, the current commit should behave well with this protocol...
There was a problem hiding this comment.
I think this applies only to the LedMatrix and the Joystick, since the Lps25h and the Hts221 have different I2C addresses.
Maybe we could have the I2cDevice instance be shared between the LedMatrix and Joystick driver, similar to what we do with the SensorDriver classes that share the same underlying device for Pressure and Temperature.
What do you think?
There was a problem hiding this comment.
I agree that we can create a class to manage the ATtiny, such that one driver represents one I2C device after all.
|
@proppy I attempted to refactor the Sense HAT driver to accommodate the shared |
|
|
||
| private static final int SENSE_HAT_REG_WHO_AM_I = 0xF0; | ||
|
|
||
| protected static I2cDevice i2cDevice; |
There was a problem hiding this comment.
instead of the static maybe we could have a ATTiny driver with 2x inner class Joystick and Display? what do you think?
I'm happy to send a pull request against your fork if you agree we should iterate in that direction.
There was a problem hiding this comment.
Yes, please send me a PR. I feel that my static approach is a bit messy as well.
There was a problem hiding this comment.
@MacroYau sorry for the delay, I'll try to send you something soon
|
Any plans to have this merged? |
|
Umm, when are we merging this? Is any help required? |
|
@rachitmishra sorry, I still have to review and make the modification suggested in #24 (comment) |
|
Please merge it soon |
This PR includes the following drivers related to the Sense HAT:
The Sense HAT metadriver has been updated accordingly. Only the Sense HAT IMU driver is missing now.
This is a revision of #22, and it has been rebased to the
masterto date.