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 to work with Bleak on a mac #78

Merged
merged 1 commit into from
Sep 20, 2020
Merged

Fix to work with Bleak on a mac #78

merged 1 commit into from
Sep 20, 2020

Conversation

finik
Copy link
Contributor

@finik finik commented Sep 13, 2020

  • Sometimes the data returnes is not byte/bytes, but native objective-c class _NSInlineData
    even though isinstance will fail, otherwise it seems to be working

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2020

Codecov Report

Merging #78 into master will decrease coverage by 0.65%.
The diff coverage is 72.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   75.76%   75.11%   -0.66%     
==========================================
  Files          18       20       +2     
  Lines        2080     2270     +190     
==========================================
+ Hits         1576     1705     +129     
- Misses        504      565      +61     
Impacted Files Coverage Δ
pylgbst/comms/cgattlib.py 0.00% <0.00%> (ø)
setup.py 0.00% <ø> (ø)
pylgbst/comms/cbluepy.py 67.81% <25.00%> (ø)
pylgbst/comms/cgatt.py 42.55% <50.00%> (-2.13%) ⬇️
pylgbst/comms/cbleak.py 51.96% <51.96%> (ø)
pylgbst/__init__.py 34.09% <62.50%> (+6.59%) ⬆️
pylgbst/hub.py 93.00% <84.61%> (-1.71%) ⬇️
pylgbst/peripherals.py 75.72% <95.65%> (+0.26%) ⬆️
tests/test_cbleak.py 97.82% <97.82%> (ø)
pylgbst/comms/__init__.py 38.31% <100.00%> (+1.22%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17ce398...9b8a988. Read the comment docs.

@finik finik mentioned this pull request Sep 13, 2020
@undera
Copy link
Owner

undera commented Sep 14, 2020

Thanks for your investigations!

I think the fix should be slightly modified, though. Instead of modifying generic parts of the library, the fix should be local to cbleak.py. Speficially around this line: https://github.com/undera/pylgbst/blob/master/pylgbst/comms/cbleak.py#L81

The reason is that generic part of lib assumes certain data types for payloads, and we should not let any implementation-specific type to slip into. So doing type conversion on msg[1] prior to calling self._handler(msg[0], msg[1]) should be the right way of fixing.

Again, I myself don't have Mac to validate this, so relying on you here...

@finik
Copy link
Contributor Author

finik commented Sep 14, 2020

I agree, I tried it but it didn't work, but there must be a way to copy bytes from _NSInlineData into real bytes and pass it over, I will clean it up

@undera
Copy link
Owner

undera commented Sep 14, 2020

Overall this looks like a bug in Bleak itself, that it leaks internal _NSInlineData class into notification function

@undera
Copy link
Owner

undera commented Sep 14, 2020

Sometimes the data returnes is not byte/bytes, but native objective-c class _NSInlineData.
Seems to be a bleak bug, just convert it to bytes as workaround for now.
@finik
Copy link
Contributor Author

finik commented Sep 19, 2020

any objections?

@undera
Copy link
Owner

undera commented Sep 20, 2020

All is good, thanks

@undera undera merged commit f015d4b into undera:master Sep 20, 2020
@finik
Copy link
Contributor Author

finik commented Sep 20, 2020

btw I don't have time to test, but it looks like the second version of the fix, which actually translated it into bytes just magically fixed all the instability issues we had before.

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