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

Add BLE scanning for S3 and C3. #5927

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Jan 26, 2022

Everything else should raise NotImplementedError.

First step in #5926

Everything else should raise NotImplementedError.

First step in micropython#5926
@tannewt tannewt added espressif applies to multiple Espressif chips ble labels Jan 26, 2022
@tannewt tannewt requested a review from dhalbert January 26, 2022 00:34
@DavePutz
Copy link
Collaborator

Tested on ESP32-S3. Advertisement worked fine, connect did throw a NotImplementedError. So, patch looks good to me.

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

This looks like it is coming along nicely! I did not test yet.

I think it would be good to add // TODO on every routine that needs implementation, or needs more implementation. That will help us spot what remains to redo.

@tannewt
Copy link
Member Author

tannewt commented Jan 26, 2022

This looks like it is coming along nicely! I did not test yet.

I think it would be good to add // TODO on every routine that needs implementation, or needs more implementation. That will help us spot what remains to redo.

My plan was to search for NotImplementedError. Is that enough?

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

I marked some places that should be flagged as NYI.

Also you can add yourself to the copyright on a bunch of files that you're doing a rewrite on,

ports/espressif/common-hal/_bleio/Characteristic.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/_bleio/CharacteristicBuffer.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/_bleio/Connection.c Outdated Show resolved Hide resolved
@dhalbert
Copy link
Collaborator

My plan was to search for NotImplementedError. Is that enough?

There are some other places too I marked.

@tannewt
Copy link
Member Author

tannewt commented Jan 27, 2022

Ah, I didn't do a bunch of functions because I just did the construct method. Will add a bunch of TODOs for those spots.

@tannewt tannewt requested a review from dhalbert January 27, 2022 01:08
@tannewt
Copy link
Member Author

tannewt commented Jan 27, 2022

Ok @dhalbert, this is ready for a re-review and passes CI.

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

Great -- thanks!

@dhalbert dhalbert merged commit fff68c9 into adafruit:main Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ble enhancement espressif applies to multiple Espressif chips
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants