-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feature: shortcut I2C support for atmel-samd boards #841
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
Conversation
Just curious what functionality this is adding? Isn't I2C already supported on these boards? |
ports/atmel-samd/board_busses.c
Outdated
|
||
STATIC mp_obj_t board_i2c(void) { | ||
#if !defined(DEFAULT_I2C_BUS_SDA) || !defined(DEFAULT_I2C_BUS_SCL) | ||
mp_raise_NotImplementedError('no default bus'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be "No default I2C bus" so that it's clear what the error relates to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. thanks
Implementing |
ports/atmel-samd/board_busses.c
Outdated
|
||
assert_pin_free(DEFAULT_I2C_BUS_SDA); | ||
assert_pin_free(DEFAULT_I2C_BUS_SCL); | ||
common_hal_busio_i2c_construct(self, DEFAULT_I2C_BUS_SCL, DEFAULT_I2C_BUS_SDA, 400000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the frequency always fixed at 400000? If so, is this what is wanted? Can this be overridden somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be over-ridden. What do you recommend? I'm still around for sprints tomorrow.
I followed what @tannewt recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can set your own fequency when you explicitly create the i2c object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah - so this is an alternative to the way we do it now? I'm just trying to understand what is being done, not objecting to it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to default to the board's standard I2C pins when they haven't been specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #340
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - thanks for clarifying - so if you omit SCL/SDA it will trigger this. Will it still accept the full set of keyword arguments in the current API? Sorry if I am just confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gives you a board.I2C
that is an instance of the default I2C bus for the board, which you can then pass to your drivers. If you need to change the pins or frequency, construct your own I2C object instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - that make sense - Thanks for explaining it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW - I think this will have a conflict with PR #829 but we can work that out
Could you please explain in the pull request message what this patch actually does? This would let us avoid confusion like the one in the recent comments. |
@deshipu yes, I've updated the PR message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the two boards without SCL and SDA can you still add the board global entry for I2C? That way they'll raise the NotImplementedError with the better message instead of a NameError.
These failures are due to PR #829 there needs to be dummy argument for the timeout.It is ignored for busio, but needed for bitbangio. I think it just needs
or if this will ever feed into bitbangio then use 255 as the default timeout for consistency. |
…obal entry for I2C? That way they'll raise the NotImplementedError with the better message instead of a NameError.
…to feature-i2c-gemma
…to feature-i2c-gemma
How do I resolve the "Changes Requested" flag? |
You resolve the issues and then the reviewer who requested the changes resolves the flag. |
@matt-land just mention me asking for a re-review. I haven't figured out what the GitHub workflow is meant to be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you!
Feature: Add default I2C buses to boards
Following a maintainer discussion in Oct '17, there was a desire to have a standardized way to initialize I2C devices when using micropython.
This PR implements a standard way to initialize I2C. It does not remove any existing methods that perform the same.
The defaults are assigned following the SDA/SCL definitions of the
'Pinout' section in /ports/amtel-sand/boards/README.rst (when available) and best effort otherwise.pins.h file of each board. A few of the boards do not have MP_QSTR_SDA/MP_QSTR_SCL defined, so I2C support was not added.Thanks to Scott
Suppored:
Not Supported: