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

_bleio: inconsistent use of linked lists vs Python lists in Service, Characteristic, and Descriptor #3228

Closed
dhalbert opened this issue Jul 30, 2020 · 1 comment · Fixed by #3310
Assignees

Comments

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 30, 2020

  • Service objects contain a field mp_obj_list_t *characteristic_list, which is meant to be a Python list of Characteristic objects.
  • Characteristic objects contain bleio_descriptor_obj_t *descriptor_list, which is meant to be a linked list of Descriptor objects.
  • Descriptor objects contain struct _bleio_descriptor_obj* next, which is used for the linked list.

But, there's this code:

mp_obj_list_append(m_desc_discovery_characteristic->descriptor_list, MP_OBJ_FROM_PTR(descriptor));

which treats descriptor_list as a Python list. This is bad. Figure out what is wrong.

I have this vague and possibly wrong memory: I think I originally used Python lists, and then decided that I should not do storage allocation in the SD event handlers, and changed to a linked list instead. But clearly this changeover was not done consistently. I think all of these could be linked lists.

The code above is only used for remote Descriptors (it's called in discovery), and the descriptor_list is used as a linked list in common_hal_bleio_characteristic_add_descriptor(), which is for local Descriptors. So perhaps that's this inconsistency is not breaking anything, but it's still not right.

@dhalbert dhalbert added this to the 6.x.x - Bug Fixes milestone Jul 30, 2020
@dhalbert dhalbert self-assigned this Aug 11, 2020
@dhalbert
Copy link
Collaborator Author

I have fixes for this that will be part of the _bleio HCI PR.

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

Successfully merging a pull request may close this issue.

1 participant