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

controller extended advertising #238

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

barbibulle
Copy link
Collaborator

@barbibulle barbibulle commented Aug 3, 2023

Enables basic support for extended advertising in the virtual controller object.

print()
print(color('LMP Features:', 'yellow'))
# TODO: support printing discrete enum values
print(' ', response.return_parameters.lmp_features.hex())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be helpful to parse features into set or IntFlag or something else because sometimes we need to check controller capabilities for both dev and product evaluation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I'll try to add that in a separate PR.

@barbibulle barbibulle force-pushed the gbg/controller-extended-advertising branch from f58fb83 to 00ed035 Compare September 26, 2023 16:45
@barbibulle barbibulle marked this pull request as ready for review November 8, 2023 01:41
)
if response.return_parameters.status == HCI_SUCCESS:
if response.return_parameters.max_page_number > 0:
print()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the empty line intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. That's to ensure a blank line before the section.



# -----------------------------------------------------------------------------
def supported_commands_as_bytes(supported_commands):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def supported_commands_as_bytes(supported_commands):
def supported_commands_as_bytes(supported_commands: Sequence[HCI_Command]) -> bytes:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -78,6 +173,25 @@ class DataObject:
pass


# -----------------------------------------------------------------------------
def le_supported_features_as_bytes(supported_features):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def le_supported_features_as_bytes(supported_features):
def le_supported_features_as_bytes(supported_features: Sequence[int]) -> bytes:

)
) is None:
return bytes([HCI_UNKNOWN_ADVERTISING_IDENTIFIER_ERROR])

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Add a TODO here to replace literals with Operation enums

else:
self.stop_advertising()
self.legacy_advertiser.enabled = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.legacy_advertiser.enabled = True
self.legacy_advertiser.enabled = False

@property
def is_advertising(self):
return self.advertising_timer_handle is not None
# Extended advertising
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should all advertisers send data on each of them was scheduled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is refactored now

self.advertising_timer_handle = asyncio.get_running_loop().call_soon(
self.on_advertising_timer_fired
# Compute the time of the next advertisement
next_advertisement = min(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This could be replaced with a heapq

# Advertiser
def send_advertising_data(self):
  ...
  heappush(timer_heap, (time.time()+interval, self))
  
def on_advertising_timer_fired(self):
  next_advertisement, advertiser = heappop(timer_heap)
  advertiser.send_advertising_data()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@zxzxwu
Copy link
Collaborator

zxzxwu commented Nov 27, 2023

By the way, as RootCanal has implemented most of the feature, can we leverage the work instead of remaking the wheels?

@barbibulle
Copy link
Collaborator Author

By the way, as RootCanal has implemented most of the feature, can we leverage the work instead of remaking the wheels?

That’s exactly the plan: the local controller class is not intended to be compete and will not be enhanced in the future. It is only there to continue support for basic local unit testing and used by a few external users who have not moved to RootCanal yet. But now that RootCanal is available as a standalone project, the plan is to use that for any sort of full-feature need.

@barbibulle barbibulle force-pushed the gbg/controller-extended-advertising branch from 23dfb1e to d12b15b Compare December 26, 2023 19:27
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.

None yet

2 participants