Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants