pbdrv/uart: Use uint32_t for read/write length #404
Conversation
|
Would it make sense to save this one for when we're closer to the end goal here? We can keep it open for now. |
|
I'm not opposed to saving it until we're closer. That being said, the very next PR (the btstack uart block impl for ev3) uses this change. Now, I am not sure on what timing basis y'all want to receive the PRs. Should I wait until I have something demonstrably working on an end-to-end basis before I send the pull requests for prerequisites? |
|
I don't see a reason to delay the change. Also, I would suggest trying it with |
|
Okay, I'll try that. I thought about it but I wasn't sure if stack space or code space was more precious. uint32_t should also be slightly faster as it avoids any extension and truncation steps on the way to and from the ALU. |
|
Adjusting the types to uint32 increased the firmware size by eight bytes. Perhaps this causes more conversions somewhere else. I decided not to change it unless there is an explicit ask to do so regardless of the firmware size. :) |
|
I'm showing on movehub (the one that matters the most) that it actually saves 20 bytes in .text at the expense of 24 bytes in .bss. This is kind of withing the "noise" level of random small changes though, so not really compelling. Before ( after ( So not really enough to say that one is better than the other. But |
|
Okay then, that makes sense! I’ll fix it up to be uint32_t when I fix the
merge conflicts. And I guess I’ll prefer the full width variables for small
structures and non arrays going forward.
…On Wed, Oct 22, 2025 at 8:55 PM David Lechner ***@***.***> wrote:
*dlech* left a comment (pybricks/pybricks-micropython#404)
<#404 (comment)>
I'm showing on movehub (the one that matters the most) that it actually
saves 20 bytes in .text at the expense of 24 bytes in .bss.
Before (uint16_t)
section size addr
.text 105360 134238208
.magic 260 536870912
.data 224 536871172
.bss 5408 536871400
.noinit 7176 536876808
.stack 3072 536883984
.name 16 134343792
.user 4 134343808
.checksum 4 134343812
.ARM.attributes 45 0
.comment 70 0
.debug_line_str 210 0
.debug_frame 116 0
Total 121965
after (uint32_t):
section size addr
.text 105340 134238208
.magic 260 536870912
.data 224 536871172
.bss 5432 536871400
.noinit 7176 536876832
.stack 3072 536884008
.name 16 134343772
.user 4 134343788
.checksum 4 134343792
.ARM.attributes 45 0
.comment 70 0
.debug_line_str 210 0
.debug_frame 116 0
Total 121969
So not really enough to say that one is better than the other. But
uint32_t seems more natural to me if there isn't a strong reason to do
something else.
—
Reply to this email directly, view it on GitHub
<#404 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGDGTAOA7HFOABUI7JLMRD3ZA7TJAVCNFSM6AAAAACJXYDE62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZUHA4DINBTHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
jaguilar
force-pushed
the
uart-buffer-length
branch
2 times, most recently
from
996da6b to
960a806
Compare
jaguilar
changed the title
btstack may send us buffers longer than 256 bytes to pass to the uart. It should be relatively low-cost to increase the length of the read and write buffers to accomodate this.
jaguilar
force-pushed
the
uart-buffer-length
branch
from
960a806 to
f039d01
Compare
|
Thanks! |
4 participants
btstack may send us buffers longer than 256 bytes to pass to the uart.
It should be relatively low-cost to increase the width of the length
argument of the read and write buffers to accommodate this.
I haven't really tested this much at all. I doubt we'll see anything with current
use cases since presumably all callsites are currently using uint8_t
as the length argument. When the btstack uart block for ev3 is done,
we may be able to exercise this a little better.
I did add some gcc pragmas in a working copy to look for conversion mistakes
in the uart implementation files. I didn't find anything in the new code.