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

fix incorrect rx buf in cdc #1810

Merged
merged 1 commit into from
Dec 22, 2022
Merged

fix incorrect rx buf in cdc #1810

merged 1 commit into from
Dec 22, 2022

Conversation

hathach
Copy link
Owner

@hathach hathach commented Dec 22, 2022

Describe the PR
This typo exists a long time ago, and it is a total mystery why tinyusb does not crash and works flawlessly so far !!

@hathach hathach merged commit 48f4d8b into master Dec 22, 2022
@hathach hathach deleted the fix-cdc-buf branch December 22, 2022 15:34
@dhalbert
Copy link
Contributor

I compiled CircuitPython with p_cdc->epout_buf and with &p_cdc->epout_buf, and the resulting .elf and .asm's are identical.

This is a peculiarity of references to C arrays: https://stackoverflow.com/questions/2528318/how-come-an-arrays-address-is-equal-to-its-value-in-c.

So though it's a little "wrong", adding the & doesn't do anything.

There's a demo program in the link above, and here's one I wrote before finding that:

#include <stdio.h>

typedef struct {
    char buf[2];
} s_t;

int main() {
    s_t s;
    s_t *sp = &s;

    void *p1 = sp->buf;
    void *p2 = &sp->buf;

    printf("p1: %p, p2: %p\n", p1, p2);
}
$ ./a.out
p1: 0x7ffcc06db176, p2: 0x7ffcc06db176

@hathach
Copy link
Owner Author

hathach commented Dec 23, 2022

thank you @dhalbert for looking deeper and detail explanation. Luckily array and &array` have the same value, C is really a mystery language :D

@RichardSWheatley
Copy link

Yes, I am familiar with C and pointers. What I have noticed is that when changes are made, there aren’t a lot of comments as to what was changed and why.

Was hoping to however made the change would put those comments here.

I use tinyusb and most of the changes that I see do not have consequences for usability.

@hathach
Copy link
Owner Author

hathach commented Feb 10, 2023

I don't understand your point. This is bug/typo and as mentioned, it should crash previously in my opinion but it doesn't (pure luck). The code speaks for itself, what else I should put the comment for ?

I use tinyusb and most of the changes that I see do not have consequences for usability.

Then don't upgrade if you have no need for those changes.

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.

4 participants