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

Issue/confusing feedback creating I2S object on Seeed XIAO RP2040 #8058

Closed
IrregularShed opened this issue Jun 2, 2023 · 13 comments · Fixed by #8466
Closed

Issue/confusing feedback creating I2S object on Seeed XIAO RP2040 #8058

IrregularShed opened this issue Jun 2, 2023 · 13 comments · Fixed by #8466
Labels
Milestone

Comments

@IrregularShed
Copy link

CircuitPython version

Adafruit CircuitPython 8.1.0 on 2023-05-22; Seeeduino XIAO RP2040 with rp2040

Code/REPL

>>> import board
>>> import audiobusio
>>> audio=audiobusio.I2SOut(bit_clock=board.D8, word_select=board.D9, data=board.D10)

Behavior

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Bit clock and word select must be sequential pins

Description

Attempting to create an I2S object in CircuitPython using the pins D8, D9 and D10 seemingly fails despite the parameters being passed matching the requirements (ie bit_clock on D8, word_select on D9).

Additional information

I took a look at the pin definitions for the XIAO RP2040 and realised that, behind the scenes, D10 comes before D9 - D10 is GPIO3, D9 is GPIO4. Swapping the word_select and data pins correctly creates an I2S object.

I guess the issue is more that the feedback you get, when trying to create the object, appears on the face of it to be wrong (when in actuality it is differently correct!). I'm not sure what the best thing to do would be to clear this up, so I thought reporting it here would pass it along in front of the eyes of a professional 😄

@tannewt tannewt added help wanted good first issue rp2 Raspberry Pi RP2 Micros labels Jun 2, 2023
@tannewt tannewt added this to the Long term milestone Jun 2, 2023
@tannewt
Copy link
Member

tannewt commented Jun 2, 2023

We could include the GPIO numbers in the error message. That may help.

@IrregularShed
Copy link
Author

That sounds sensible!

@todbot
Copy link

todbot commented Jun 12, 2023

Hi @IrregularShed have you tried the "Where's My I2S?" script in the Learn Guide? It's pretty helpful.

For the Xiao RP2040, using the above script, I've found that this works:

audio=audiobusio.I2SOut(bit_clock=board.SCK, word_select=board.MOSI, data=board.MISO)

this is equivalent to:

audio=audiobusio.I2SOut(bit_clock=board.D8, word_select=board.D10, data=board.D9)

Unfortunately, when audiobusio is complaining about sequential pins, it's talking about RP2040 GPIO pins, not board.* pins, which can be arbitrary and varies from board to board.

@IrregularShed
Copy link
Author

@todbot Yep, I've used that in the past with a Pi Pico and ESP32-S2 board. I can't believe I didn't think about using it this time...

Funnily enough my build uses the exact same pins as you mentioned (not that there's many to choose from on a Xiao!) and it works wonderfully. I used some code that you wrote as a basis and ran with it; I've fallen into the Eurorack universe so it's ended up with a pair of CV inputs, a pair of pots and a rotary encoder. It's in the middle here, and I apologise for going off topic but I'm happy with it!

20230608_180040

@todbot
Copy link

todbot commented Jun 13, 2023

@IrregularShed that's very cool! Is this issue closeable for you?
I'm not sure how rp2040 audiobusio error message could give a more actionable message to the user, since there's such a disintermediation between microcontroller.pin.* and board.*

@dhalbert
Copy link
Collaborator

ValueError: Bit clock and word select must be sequential pins

If this said

ValueError: Bit clock and word select must be sequential GPIO pins

would that have been clearer?

@IrregularShed
Copy link
Author

@dhalbert I think that would make it clearer, yeah. Just adding GPIO would've given me a hint where to look a bit earlier.

@heygauri
Copy link

heygauri commented Oct 4, 2023

@dhalbert @IrregularShed I'm currently working on the mentioned issue, but I'm facing confusion regarding which specific file needs to be modified. I have identified multiple audiobusio files. Could you please provide clarity on the exact file that requires modification for this issue?

sumitra@sumitra:~/opensource/circuitpython$ git grep "Bit clock and word select must be sequential pins" .
locale/ID.po:msgid "Bit clock and word select must be sequential pins"
locale/circuitpython.pot:msgid "Bit clock and word select must be sequential pins"
locale/cs.po:msgid "Bit clock and word select must be sequential pins"
locale/de_DE.po:msgid "Bit clock and word select must be sequential pins"
locale/el.po:msgid "Bit clock and word select must be sequential pins"
locale/en_GB.po:msgid "Bit clock and word select must be sequential pins"
locale/en_GB.po:msgstr "Bit clock and word select must be sequential pins"
locale/es.po:msgid "Bit clock and word select must be sequential pins"
locale/fil.po:msgid "Bit clock and word select must be sequential pins"
locale/fr.po:msgid "Bit clock and word select must be sequential pins"
locale/hi.po:msgid "Bit clock and word select must be sequential pins"
locale/it_IT.po:msgid "Bit clock and word select must be sequential pins"
locale/ja.po:msgid "Bit clock and word select must be sequential pins"
locale/ko.po:msgid "Bit clock and word select must be sequential pins"
locale/nl.po:msgid "Bit clock and word select must be sequential pins"
locale/pl.po:msgid "Bit clock and word select must be sequential pins"
locale/pt_BR.po:msgid "Bit clock and word select must be sequential pins"
locale/ru.po:msgid "Bit clock and word select must be sequential pins"
locale/sv.po:msgid "Bit clock and word select must be sequential pins"
locale/tr.po:msgid "Bit clock and word select must be sequential pins"
locale/zh_Latn_pinyin.po:msgid "Bit clock and word select must be sequential pins"
ports/raspberrypi/common-hal/audiobusio/I2SOut.c: mp_raise_ValueError(translate("Bit clock and word select must be sequential pins"));

sumitra@sumitra:~/opensource/circuitpython$ git grep "audiobusio" .
docs/redirects.txt:shared-bindings/audiobusio/I2SOut.rst shared-bindings/audiobusio/#audiobusio.I2SOut
docs/redirects.txt:shared-bindings/audiobusio/PDMIn.rst shared-bindings/audiobusio/#audiobusio.PDMIn
docs/redirects.txt:shared-bindings/audiobusio/init.rst shared-bindings/audiobusio/
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/espressif/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/nrf/common-hal/audiobusio/I2SOut.c ports/nrf/common-hal/rtc/RTC.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: shared-bindings/audiobusio/I2SOut.c shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/mimxrt10xx/common-hal/audiobusio/init.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/nrf/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/mimxrt10xx/common-hal/audiobusio/init.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: shared-bindings/audiobusio/I2SOut.c shared-bindings/audioio/AudioOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/espressif/common-hal/audiobusio/init.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/raspberrypi/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/I2SOut.c
locale/ID.po:#: ports/atmel-samd/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: shared-bindings/audiobusio/PDMIn.c
locale/ID.po:#: ports/nrf/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: ports/stm/common-hal/audiobusio/PDMIn.c
locale/ID.po:#: ports/stm/common-hal/
....

PS: I am new to this project :)

@elpekenin
Copy link

You most likely want to look at the .c file with translate("...") on it. There are also a ton of files (one per each translation) which contain the translated string.

@heygauri
Copy link

heygauri commented Oct 4, 2023

The only ".c " file with "translate" I see is this "ports/raspberrypi/common-hal/audiobusio/I2SOut.c: mp_raise_ValueError(translate("Bit clock and word select must be sequential pins"));"

Do you think that it is the required file?

@dhalbert
Copy link
Collaborator

dhalbert commented Oct 4, 2023

@heygauri Yes, just edit the message in quotes there inside the translate(). Then do a make translate at the top level, to get that into the circuitpython.pot file. Do not in general edit the files in locale directly: circuitpython.pot is produced by make translate, and the other languages are modified by weblate.

@heygauri
Copy link

heygauri commented Oct 4, 2023

@dhalbert Sure, I will do this.

heygauri added a commit to heygauri/circuitpython that referenced this issue Oct 8, 2023
The error message for creating an I2S object on the rp2040 platform
in CircuitPython can be misleading when the word_select and data pins
are not sequential. This change updates the error message to provide
clearer guidance by specifying "GPIO pins" instead of just "pins".
The revised message now reads:

ValueError: Bit clock and word select must be sequential GPIO pins

Closes adafruit#8058

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
@heygauri
Copy link

heygauri commented Oct 8, 2023

@dhalbert I am not sure why I am failing the tests in the PR #8466. I exactly followed the steps you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants