Skip to content

fix: Darwin services ordering in DiscoverServices #417

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.

Sign up for GitHub

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

Merged
deadprogram merged 2 commits into from
Mar 17, 2026
Merged

fix: Darwin services ordering in DiscoverServices #417

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3f30d0
fix: Darwin services ordering in DiscoverServices
acouvreur Mar 4, 2026
8df11be
remove pointer
acouvreur Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 34 additions & 18 deletions gattc_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package bluetooth

import (
"errors"
"slices"
"time"

"github.com/tinygo-org/cbgo"
Expand All @@ -24,33 +25,34 @@ func (d Device) DiscoverServices(uuids []UUID) ([]DeviceService, error) {
select {
case <-d.servicesChan:
svcs := []DeviceService{}

if len(uuids) > 0 {
// The caller wants to get a list of services in a specific
// order.
svcs = make([]DeviceService, len(uuids))
}
Comment on lines 27 to +33
Copy link
Contributor

@HattoriHanzo031 HattoriHanzo031 Mar 16, 2026

Choose a reason for hiding this comment

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

This could be simplified:

Suggested change
svcs := []DeviceService{}
if len(uuids) > 0 {
// The caller wants to get a list of services in a specific
// order.
svcs = make([]DeviceService, len(uuids))
}
svcs := make([]DeviceService, len(uuids))

Copy link
Author

@acouvreur acouvreur Mar 16, 2026

Choose a reason for hiding this comment

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

I've used the same "algorithm" that was already implemented in other OS specific.

Should we change it in this PR ?

Should we change all algorithms ?

Copy link
Member

@deadprogram deadprogram Mar 16, 2026

Choose a reason for hiding this comment

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

I've used the same "algorithm" that was already implemented in other OS specific.

Should we change it in this PR ?

Should we change all algorithms ?

Perhaps we should look into seeing if we can use shared/improved code for all of them, but probably not in this PR.


for _, dsvc := range d.prph.Services() {
dsvcuuid, _ := ParseUUID(dsvc.UUID().String())
// add if in our original list

// only include services that are included in the input filter
if len(uuids) > 0 {
found := false
for _, uuid := range uuids {
for j, uuid := range uuids {
if dsvcuuid.String() == uuid.String() {
// one of the services we're looking for.
found = true
break
// One of the services we're looking for.
svcs[j] = d.makeService(dsvcuuid, dsvc)
Copy link
Contributor

@HattoriHanzo031 HattoriHanzo031 Mar 16, 2026

Choose a reason for hiding this comment

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

the loop should probably break as soon as the service uuid is found, but this raises the question if duplicate uuids are allowed in the input of the method (which documentation does not prevent and the code does not check for).

Copy link
Member

@deadprogram deadprogram Mar 16, 2026

Choose a reason for hiding this comment

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

AFAIK duplicates should be allowed.

}
}
if !found {
continue
}
} else {
// The caller wants to get all services, in any order.
svcs = append(svcs, d.makeService(dsvcuuid, dsvc))
}
Comment on lines +46 to 49
Copy link
Contributor

@HattoriHanzo031 HattoriHanzo031 Mar 16, 2026

Choose a reason for hiding this comment

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

maybe move this case to the beginning of the loop for clarity

			if len(uuids) == 0 {
				// The caller wants to get all services, in any order.
				svcs = append(svcs, d.makeService(dsvcuuid, dsvc))
				continue
			}

}

svc := DeviceService{
deviceService: &deviceService{
uuidWrapper: dsvcuuid,
device: d,
service: dsvc,
},
}
svcs = append(svcs, svc)
d.services[svc.uuidWrapper] = svc
if slices.Contains(svcs, (DeviceService{})) {
return nil, errors.New("bluetooth: did not find all requested services")
}

return svcs, nil
case <-time.NewTimer(10 * time.Second).C:
return nil, errors.New("timeout on DiscoverServices")
Expand All @@ -61,6 +63,20 @@ func (d Device) DiscoverServices(uuids []UUID) ([]DeviceService, error) {
// struct method of the same name.
type uuidWrapper = UUID

// Small helper to create a DeviceService object.
func (d Device) makeService(dsvcuuid uuidWrapper, dsvc cbgo.Service) DeviceService {
svc := DeviceService{
deviceService: &deviceService{
uuidWrapper: dsvcuuid,
device: d,
service: dsvc,
},
}
// Cache the service in the device's services map, so that we can find it
d.services[svc.uuidWrapper] = svc
return svc
}

// DeviceService is a BLE service on a connected peripheral device.
type DeviceService struct {
*deviceService // embdedded as pointer to enable returning by []value in DiscoverServices
Expand Down
Loading