-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added definition for an upcoming Pimoroni motor driver board #6314
Conversation
You could do that, as you did here: #6208 (comment). The main issue is to make sure that if a static object has pointers to heap objects, that those pointers are included in garbage collection. |
Thanks. I'm a bit pushed for time at the moment, so if I do add those tuples it's unlikely to be for this PR |
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 fine! I had a query/suggestion.
{ MP_ROM_QSTR(MP_QSTR_CURRENT_SENSE_A_ADDR), MP_ROM_INT(0b000) }, | ||
{ MP_ROM_QSTR(MP_QSTR_CURRENT_SENSE_B_ADDR), MP_ROM_INT(0b001) }, | ||
{ MP_ROM_QSTR(MP_QSTR_CURRENT_SENSE_C_ADDR), MP_ROM_INT(0b010) }, | ||
{ MP_ROM_QSTR(MP_QSTR_CURRENT_SENSE_D_ADDR), MP_ROM_INT(0b011) }, | ||
{ MP_ROM_QSTR(MP_QSTR_VOLTAGE_SENSE_ADDR), MP_ROM_INT(0b100) }, | ||
{ MP_ROM_QSTR(MP_QSTR_FAULT_SENSE_ADDR), MP_ROM_INT(0b101) }, | ||
{ MP_ROM_QSTR(MP_QSTR_SENSOR_1_ADDR), MP_ROM_INT(0b110) }, | ||
{ MP_ROM_QSTR(MP_QSTR_SENSOR_2_ADDR), MP_ROM_INT(0b111) }, | ||
{ MP_ROM_QSTR(MP_QSTR_NUM_SENSORS), MP_ROM_INT(2) }, |
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.
If there is going to be a library you (almost) always use with this board, you might consider putting constants like these and NUM_MOTORS
in the library rather than casting them in stone.
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 okay. There's no specific library for this board at the moment, I was just going to use the existing motor,encoder,neopixel etc libraries, as my time is stretched pretty thin with supporting C++ and Micropython. Something to consider for the future though.
For your reference, another case of putting a tuple of things in the
|
Thanks for that @jepler. Which file is that in, for reference? |
oops meant to leave the reference in the comment:
|
Follow up from a similar fix in 426785a Fixes issue adafruit#6314. Signed-off-by: Damien George <damien@micropython.org>
Let me know if you spot any issues. Hoping to launch at the end of the week.
Also I included a NEOPIXEL alias as this was highlighted as missing on Servo 2040 (#6307)
I did also consider including constant tuples for the motor and encoder pairs, as I recently discovered they could be done in MicroPython, but this seemed a bit experimental to include in these CircuitPython definitions.