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

seeed interface: fix fileno issue with windows #902

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

Gussy
Copy link
Contributor

@Gussy Gussy commented Aug 26, 2020

Environment:

  • Python: 3.8.2
  • Windows-10-10.0.18362-SP0

Issue
When using the seeedstudio interface on Linux or MacOS, I'm able to connect to the interface and send/receive messages.
When running the same script on Windows 10 the following error is thrown:

Traceback (most recent call last):
  File "<redacted>.py", line 264, in <module>
  File "<redacted>.py", line 44, in __init__
  File "can\notifier.py", line 56, in __init__
  File "can\notifier.py", line 66, in add_bus
  File "can\interfaces\seeedstudio\seeedstudio.py", line 266, in fileno
io.UnsupportedOperation: fileno

From the python-serial docs, fileno() is only supported on Posix platforms.

The commit below fixes the issue for me, without breaking functionality on Linux, although I'm not sure if this is correct solution.

@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@7f1549f). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             develop     #902   +/-   ##
==========================================
  Coverage           ?   68.37%           
==========================================
  Files              ?       71           
  Lines              ?     6992           
  Branches           ?        0           
==========================================
  Hits               ?     4781           
  Misses             ?     2211           
  Partials           ?        0           

@bmeisels
Copy link
Contributor

This has come up also with with slcan (#760).
and the solution chosen was to raise an error which will be caught by the notifier(#877).
Suggest you revise your solution in line with the change to notifier as i believe your current patch changes the behavior on Linux.
I would consider detecting explicitly if you are on windows.

@Gussy Gussy force-pushed the gussy/fix-fileno branch 3 times, most recently from 0285c31 to 83dea3f Compare August 28, 2020 00:41
@Gussy
Copy link
Contributor Author

Gussy commented Aug 28, 2020

Thanks for the feedback @bmeisels.

I think that I understood what you're asking for based on those two PRs you linked to, so I went ahead and amended the commit.

# Return an invalid file descriptor on Windows
except io.UnsupportedOperation:
raise NotImplementedError("fileno is not implemented using current CAN bus")

return -1
Copy link
Owner

Choose a reason for hiding this comment

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

This line now looks unreachable, and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hardbyte I've amended the commit to remote that line 👍

@Gussy Gussy force-pushed the gussy/fix-fileno branch 2 times, most recently from 816bdd9 to f1f7fe3 Compare August 31, 2020 06:23
Copy link
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Thanks!

Sorry it took me a long time to get back to this

@Gussy
Copy link
Contributor Author

Gussy commented Oct 6, 2020

Is there anything I can do to help get this merged into develop?

@mergify mergify bot merged commit 833e50e into hardbyte:develop Oct 7, 2020
@Gussy Gussy deleted the gussy/fix-fileno branch October 7, 2020 16:43
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.

4 participants