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

Event dispatch bugfix #604

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Event dispatch bugfix #604

merged 1 commit into from
Dec 10, 2020

Conversation

orrmany
Copy link
Contributor

@orrmany orrmany commented Dec 4, 2020

Hi, here comes a bugfix for a missing dereference. Combined with the headache of migration to PlatformIO Core 5.x, this caused me several hours frustration..
Probably I made the original erroneous pull-request into 0.21.0 from the beginning....

@orrmany
Copy link
Contributor Author

orrmany commented Dec 4, 2020

@hathach, maybe we should rename Bluefruit._mprot_event_sem into Bluefruit._mprot_event_sem_p to prevent similar errors?

@orrmany
Copy link
Contributor Author

orrmany commented Dec 4, 2020

Well, I found the commit, which introduced the bug: ff12326 . I made this somehow while I made editorial changes :(

@hathach
Copy link
Member

hathach commented Dec 7, 2020

@hathach, maybe we should rename Bluefruit._mprot_event_sem into Bluefruit._mprot_event_sem_p to prevent similar errors?

the semaphore is the pointer itself, maybe you could just use SemaphoreHandle_t to declare _mprot_event_sem and setMultiprotocolSemaphore

@orrmany
Copy link
Contributor Author

orrmany commented Dec 7, 2020 via email

@hathach
Copy link
Member

hathach commented Dec 7, 2020

semaphore handle is pointer itself, there is no need to handle it within the bluefruit, just set(handle) when created, and set(NULL) before delete by application would work.

@orrmany
Copy link
Contributor Author

orrmany commented Dec 7, 2020

semaphore handle is pointer itself, there is no need to handle it within the bluefruit, just set(handle) when created, and set(NULL) before delete by application would work.

Hmm, that would mean some API change to yet another library I am developing, but maybe your are right. I will come back to you soon

@hathach
Copy link
Member

hathach commented Dec 7, 2020

Hmm, that would mean some API change to yet another library I am developing, but maybe your are right. I will come back to you soon

If you don't want to change your lib, that is ok. We can leave it as it is now. Let me know what you decide to do

@orrmany
Copy link
Contributor Author

orrmany commented Dec 9, 2020

Hi @hathach, I have implemented your proposal: as part of the bugfix I made _mprot_event_sem a regular semaphore (and also rebased the bugfix for cleanup)

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

look good

@hathach hathach merged commit be4d31c into adafruit:master Dec 10, 2020
@orrmany orrmany deleted the patch-1 branch December 17, 2020 13:30
orrmany added a commit to orrmany/SDAntplus that referenced this pull request Dec 17, 2020
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