Skip to content

seeed interface: fix fileno issue with windows#902

Merged
mergify[bot] merged 2 commits intohardbyte:developfrom
Gussy:gussy/fix-fileno
Oct 7, 2020
Merged

seeed interface: fix fileno issue with windows#902
mergify[bot] merged 2 commits intohardbyte:developfrom
Gussy:gussy/fix-fileno

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.

@Gussy Gussy force-pushed the gussy/fix-fileno branch from 83dea3f to 16a3a10 Compare August 28, 2020 00:48
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
@Gussy Gussy force-pushed the gussy/fix-fileno branch from f1f7fe3 to 77c1737 Compare August 31, 2020 06:27
@Gussy Gussy requested a review from hardbyte September 10, 2020 02:16
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