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

Set ScanResponse to off so the advertising will actually advertise... #49

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Set ScanResponse to off so the advertising will actually advertise... #49

merged 1 commit into from
Oct 20, 2020

Conversation

aovestdipaperino
Copy link
Contributor

…the name.
See this line for why.

@T-vK
Copy link
Owner

T-vK commented Oct 16, 2020

Thanks for the PR, before I can accept it, it needs to be tested on Mac OS, Windows, Linux Android and iOS. What operating systems have you tested this with?

@aovestdipaperino
Copy link
Contributor Author

MacOS, iOS and Android. I might be able to help with Windows and Chrome OS (which should cover Linux?)

@aovestdipaperino
Copy link
Contributor Author

Also: irrelevant to the topic, but the latest release zip didn't seem to match the code in the repository. For example the BleKeyboard::onConnected(BLEServer *pServer) was missing.

@T-vK
Copy link
Owner

T-vK commented Oct 17, 2020

I don't know about Chrome OS, but it would be nice to have it tested there as well. I will test it on a standard Linux distro next week and if you or someone else confirms it also works on Windows, I'll accpt the PR.
Yeah I should have made a new release after onConnected was added. I'll make a new release when your PR is merged.

@aovestdipaperino
Copy link
Contributor Author

Awesome. I will do it on Windows, I can borrow a laptop.
One courtesy: I made a mistake with user when I submitted. If you don't mind I will close this one and reopen a new identical one.

@T-vK
Copy link
Owner

T-vK commented Oct 17, 2020

Normally you would just undo your commit and then make a new commit and force push it. This would automatically modify the PR. E.g.

git reset HEAD^
git add .
git commit -m "Set ScanResponse......."
git push --force

but if you find that's too complicated, feel free to make another PR.

@aovestdipaperino
Copy link
Contributor Author

Done thanks. I didn't realize it was the last commit in this repository as I had a similar problem in another one.

@T-vK
Copy link
Owner

T-vK commented Oct 20, 2020

I did a quick Linux test and it seems to work. When you've verified it works on Windows, I'll merge.

@aovestdipaperino
Copy link
Contributor Author

All done, looks legit. Thank you, this was smooth.

@T-vK T-vK merged commit 92a056a into T-vK:master Oct 20, 2020
@T-vK
Copy link
Owner

T-vK commented Jan 18, 2021

Probably yes, but I'm not sure how this has affected iOS/MacOS compatibility. It seems like some of the most recent commits broke compatibility with certain iPhones/iPads and at the same time fix issues for other iPhones/iPads. This kinda makes me wanna drop Apple support completely...

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