Skip to content

Update pins.c for m5dial #9404

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

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Update pins.c for m5dial #9404

merged 2 commits into from
Jul 9, 2024

Conversation

mumin50
Copy link

@mumin50 mumin50 commented Jul 4, 2024

Hi All my first pull request ever :)
I just realised i miss SDA and SCL pins for M5Dial NFC reader, it is connected directly to board.I2C but in some usages having those pins will make things easier to work with busio

Missing SDA and SCL pins for M5Dial NFC reader
@mumin50
Copy link
Author

mumin50 commented Jul 4, 2024

Tested on my side - compiled and working ok
`Adafruit CircuitPython 9.1.0-beta.4-2-gc64bdf2c74 on 2024-07-04; M5Stack Dial with ESP32S3

import board
dir(board)
['class', 'name', 'A1', 'A2', 'BOOT0', 'BUTTON', 'D0', 'D1', 'D13', 'D15', 'D2', 'DISPLAY', 'ENC_A', 'ENC_B', 'I2C', 'KNOB_BUTTON', 'MOSI', 'NEOPIXEL', 'PORTA_I2C', 'PORTB_IN', 'PORTB_OUT', 'POWER_HOLD', 'RFID_IRQ', 'RFID_SCL', 'RFID_SDA', 'SCK', 'SCL', 'SDA', 'SPEAKER', 'SPI', 'TFT_BACKLIGHT', 'TFT_CS', 'TFT_DC', 'TFT_RESET', 'TOUCH_IRQ', 'dict', 'board_id']
`

Tested with my library code - also working
`code.py output:
i2c initialized
Place card before reader to read from address 0x08
New card detected

  • tag type: 0x10
  • uid : 0x8804851b`

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I'm not sure of the naming here. board.PORTA_I2C() uses the GPIO13/15 for its SDA/SCL. But those pins are board.SDA and board.SCL. Normally board.I2C would use board.SDA and board.SCL. But here board.I2C() uses board.RFID_SDA and board.RFID_SCL.

From mpconfigboard.h:

#define CIRCUITPY_BOARD_I2C_PIN     {{.scl = &pin_GPIO12, .sda = &pin_GPIO11}, \
                                     {.scl = &pin_GPIO15, .sda = &pin_GPIO13}}

Are board.RFID_SDA and board.RFID_SCL broken out for general use, or are they strictly internal for the RFID? If the latter, then I'd suggest board.RFID_I2C() for them, and board.I2C() for the Port A SDA and SCL instead.

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 4, 2024

@CDarius You did the original naming. What do you think?

@CDarius
Copy link

CDarius commented Jul 5, 2024

The board.I2C() (pins 11 and 12) is internal (not broken out) but it serves two peripherals: RFID and RTC.
I agree that having pin 13 board.SDA and pin 15 board.SCL on board.PORTA_I2C() may be confusing.
I suggest the following changes:

  • Rename board.SDA in board.PORTA_SDA (pin 13)
  • Rename board.SCL in board.PORTA_SCL (pin 15)
  • Add board.SDA on pin 11
  • Add board.SCL on pin 12

@mumin50
Copy link
Author

mumin50 commented Jul 5, 2024

I just also realised it is used for other devices on I2C so probably naming is wrong. Sorry i was only using RFID for now so perchaps like CDarius mentioned would be better.
Just tell when i gladly do the testing .

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 9, 2024

@CDarius' suggestion makes sense to me, as it is consistent with m5stack_paper and some other m5stack boards. There are also some examples of a different naming scheme with SYS_i2C, but there are more boards with the plain I2C() as the internal one, so we can stick with that.

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 9, 2024

I made the changes. @mumin50 and @CDarius, could you double-check that this is OK?

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I double-checked so I'll go ahead and merge.

@dhalbert dhalbert merged commit 353174c into adafruit:main Jul 9, 2024
15 checks passed
@CDarius
Copy link

CDarius commented Jul 12, 2024

@dhalbert I little late but I looked at your changes and they look good to me

@mumin50 mumin50 deleted the patch-1 branch August 7, 2024 17:27
@mumin50
Copy link
Author

mumin50 commented Aug 7, 2024

Works ok thank You

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.

3 participants