Skip to content

Adds new error message for SAMD busio UART RX buffer size#3902

Closed
AdamCummick wants to merge 0 commit into
adafruit:mainfrom
AdamCummick:main
Closed

Adds new error message for SAMD busio UART RX buffer size#3902
AdamCummick wants to merge 0 commit into
adafruit:mainfrom
AdamCummick:main

Conversation

@AdamCummick
Copy link
Copy Markdown

Raises an error when a busio UART "receiver_buffer_size" is not a multiple of 4. Behavior referenced in issue #3901 and tested using same setup: SAMD51 and CircuitPython 6.0.0

Example:

import board
import busio

buffer_len = 5 # will fail unless this is a multiple of 4
port1 = busio.UART(board.TX1, board.RX1, baudrate=115200, receiver_buffer_size=buffer_len)

Will raise an error with the message: ValueError: Invalid RX buffer size of 5, must be a multiple of 4

@dhalbert
Copy link
Copy Markdown
Collaborator

Thanks for noticing this. I think we should explore the causation behind the error: whether SAMD actually requires this or we are doing something wrong somewhere.

We suggest creating a branch in your forked repo, instead of changing your own main. That causes your main to get out of sync with the upstream main. See https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/always-work-on-a-branch.

Copy link
Copy Markdown
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.

See non-review comment as well.

}

if (receiver_buffer_size % 4 != 0) {
mp_raise_msg_varg(&mp_type_ValueError, translate("Invalid RX buffer size of %d, must be a multiple of 4"), receiver_buffer_size);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a long message, and we need to save space. We don't need to print out the input argument, so I'd suggest this (using the argument name in shared-bindings/busio/UART.c):

Suggested change
mp_raise_msg_varg(&mp_type_ValueError, translate("Invalid RX buffer size of %d, must be a multiple of 4"), receiver_buffer_size);
mp_raise_ValueErorr( translate("receiver_buffer_size must be a multiple of 4"), receiver_buffer_size);

@AdamCummick
Copy link
Copy Markdown
Author

Thanks for noticing this. I think we should explore the causation behind the error: whether SAMD actually requires this or we are doing something wrong somewhere.

It looks like it's based on the requirements of the ATMEL hal ringbuffer: https://github.com/adafruit/asf4/blob/84f56af13292d8f32c40acbd949bde698ddd4507/samd51/hal/utils/src/utils_ringbuffer.c#L53

Looks like my assumptions were wrong anyhow, power of 2 instead of multiples of 4.

@dhalbert
Copy link
Copy Markdown
Collaborator

It looks like it's based on the requirements of the ATMEL hal ringbuffer: https://github.com/adafruit/asf4/blob/84f56af13292d8f32c40acbd949bde698ddd4507/samd51/hal/utils/src/utils_ringbuffer.c#L53

Looks like my assumptions were wrong anyhow, power of 2 instead of multiples of 4.

We might consider rounding up to the nearest power of 2, and changing the documentation to note that the buffer size might be bigger. And/or noting that some platforms might have specific requirements.

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.

2 participants