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

fix advertising parameter usage for legacy advertising #633

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

Conversation

markusjellitsch
Copy link
Contributor

when using start_adverting() the usage of advertising_interval_min and advertising_interval_max for legacy advertising is not consistent with extended advertising (The advertising_interval given as input param should be calculated in 0.625 units)

@zxzxwu
Copy link
Collaborator

zxzxwu commented Jan 23, 2025

This API has been widely used, so we shouldn't roughly alter its spec. At least we should check its type to determine whether it should be considered 1 or 0.625 ms.

@markusjellitsch
Copy link
Contributor Author

Yes understand. At the moment it's a bit confusing and inconsistent, when you call start_advertising()and legacy adv is used it is 0.625 ms, for extended adv it is 1.

@barbibulle
Copy link
Collaborator

I think the change here makes sense. It was never the intention of the API to accept values in units of 0.625 (as a general rule, the Device API tries to hide internal spec details). All those intervals are supposed to be in milliseconds. In fact, for extended advertising, that's what the implementation does.
So this should be considered a bug fix, not an API change.

@barbibulle
Copy link
Collaborator

@markusjellitsch could you run the formatter (check with invoke project.pre-commit for details) so that the linter/formatter checks can pass before we merge the change?

@markusjellitsch
Copy link
Contributor Author

@barbibulle I reverted advertising_interval to float.

@markusjellitsch
Copy link
Contributor Author

I checked with invoke project.pre-commit and this seems fine (see log below). However Github Actions v3 seems deprecated from now on. Is there anything I can do from my side.

`device_test.py::test_cis
device_test.py::test_cis_setup_failure
/home/markus/projects/bumble/bumble/utils.py:467: FutureWarning: Only for testing
warnings.warn(msg, FutureWarning)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================== 347 passed, 1 skipped, 7 warnings in 8.17s ==================
All good!`

@barbibulle
Copy link
Collaborator

I'll need to update the GitHub actions, looks like some actions have become obsolete. I'll get to it now.

@barbibulle
Copy link
Collaborator

Ah, looks like you've already done that :-)

bumble/device.py Outdated
@@ -2587,8 +2587,8 @@ async def start_advertising(
auto_restart: bool = False,
advertising_data: Optional[bytes] = None,
scan_response_data: Optional[bytes] = None,
advertising_interval_min: Optional[float] = None,
advertising_interval_max: Optional[float] = None,
advertising_interval_min: Optional[int] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keep float here? While not strictly necessary, I think it's Ok to let the caller pass in non-integer millisecond values, since the spec supports it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it because of pylint expecting int. It would require more code changes (adapt all internal advertising interval member variables to non-int). In the long run I'd definetely allow the caller to set the complete range of allowed values.

I am not sure if this change should be in the scope of this PR :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave like as int for now. I thought that float was supposed to imply that you could pass an int too (quoting https://typing.readthedocs.io/en/latest/spec/special-types.html#special-cases-for-float-and-complex ):

Special cases for float and complex
Python’s numeric types complex, float and int are not subtypes of each other, but to support common use cases, the type system contains a straightforward shortcut: when an argument is annotated as having type float, an argument of type int is acceptable; similar, for an argument annotated as having type complex, arguments of type float or int are acceptable.

But maybe pylint isn't following that rule. We can leave that for a separate PR.

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.

3 participants