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

SocketcandMedia Media class added for socketcand functionality #306

Merged
merged 49 commits into from
Sep 8, 2023

Conversation

wiboticanders
Copy link
Contributor

Although this runs on PythonCAN, changing the PythonCANMedia class to function with socketcand is not very clean because socketcand has 2 additional variables compared to all the other PythonCAN compatible interfaces on pycyphal.
Socketcand can be referenced here: https://github.com/linux-can/socketcand , it is used for remote CAN access of udp socket. As far as copyright goes, because this is very similar to the PythonCANMedia class and I reused code from it I believe that the copyright still applies.

the old implimentation to a new one
added another error check for socketcand
and CAN FD at same time
_InterfaceParameters instead of separately in the
FD and classic subclasses
This reverts commit d9f8267.
library functionality. It runs on Python-CAN
like the interfaces in the PythonCANMedia class,
but because it has extra variables (host and port),
it is not very clean to just add socketcand to
PythonCANMedia.
@wiboticanders
Copy link
Contributor Author

Hello, can't tell why the read the docs build failed. Maybe because I added new files?

@maksimdrachov
Copy link
Member

Regarding testing, it might be useful to take a look at the following forum post:

https://forum.opencyphal.org/t/development-setup-pycyphal-ubuntu/1844

As it relates to your question:

  • You can run the docs build/test using the following command (after the setup is done):
nox --session docs
  • The unit tests can also be run for each Python version seperately:
nox --session test-3.10

(You can also select a subset of tests to be run, see link above: Section 3, Step 5)

If you have any questions regarding the setup: feel free to ask them in the forum post! :)

@pavel-kirienko
Copy link
Member

can't tell why the read the docs build failed.

Please check the logs:

image

They say:

/home/docs/checkouts/readthedocs.org/user_builds/pycyphal/checkouts/306/pycyphal/transport/can/media/socketcand/_socketcand.py:docstring of pycyphal.transport.can.media.socketcand._socketcand.SocketcandMedia:18: WARNING: Literal block ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/pycyphal/checkouts/306/pycyphal/transport/can/media/socketcand/_socketcand.py:docstring of pycyphal.transport.can.media.socketcand._socketcand.SocketcandMedia.__init__:15: WARNING: Unexpected indentation.
/home/docs/checkouts/readthedocs.org/user_builds/pycyphal/checkouts/306/pycyphal/transport/can/media/socketcand/_socketcand.py:docstring of pycyphal.transport.can.media.socketcand._socketcand.SocketcandMedia.__init__:16: WARNING: Block quote ends without a blank line; unexpected unindent.

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

If I understand how it's built correctly, we no longer need the _construct_socketcand handler, the classic/FD differentiation (set the fd flag in the constructor when creating ThreadSafeBus), and the auxiliary types like PythonCANBusOptions and _InterfaceParameters. Removing these should simplify the implementation quite a bit.

Please format the code correctly using Black (more on this in CONTRIBUTING.md).

The old Yakut examples with --transport should be removed because this is no longer the recommended usage pattern.

This fixes the unnecessary pieces left from the pythonCANMedia
class, such as the FD vs Classic dataclasses, Can Bus Options,
and the constructor class. It also updates the way
parameters are inputed to be all in the iface_name
param like 'socketcand:can:host:port'
@pavel-kirienko
Copy link
Member

pavel-kirienko commented Jul 14, 2023

Please try adding a simple integration test for this media implementation. You can make a new test case similar to the SocketCAN test that starts a local socketcand daemon and connects to it.

Added unit test and changed spec string to be parsed in _transport_factory

fixed formatting issues that did not pass Black guidelines

Removed unecessary params S

Updated version of the socketcandMedia class. This fixes the unnecessary pieces left from the pythonCANMedia class, such as the FD vs Classic dataclasses, Can Bus Options, and the constructor class. It also updates the way parameters are inputed to be all in the iface_name param like 'socketcand:can:host:port'

SocketcandMedia Media class added for socketcand library functionality. It runs on Python-CAN like the interfaces in the PythonCANMedia class, but because it has extra variables (host and port), it is not very clean to just add socketcand to PythonCANMedia.
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
@pavel-kirienko
Copy link
Member

You should now be able to run the test suite by pushing a commit with the #test hashtag in the commit message.

Test environment now properly builds and breaks down socketcand
Tests use rx_a and rx_b for proper communication
@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 30, 2023 via email

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 30, 2023 via email

@wiboticanders
Copy link
Contributor Author

wiboticanders commented Aug 30, 2023 via email

@pavel-kirienko
Copy link
Member

So possibly something like this within workflow

Yes!

And then within the actual unit test I can run the daemon and take it down

image

Also modified docs of SocketcandMedia, and updated socketcandMedia
test to only start the daemon instead of building socketcand, setting
up a vcan, and then running the daemon, now socketcand is installed
in workflow and we use an existing vcan set up previously.
I also added some error handling for running the socketcand
daemon and made sure to kill the process correctly
@wiboticanders
Copy link
Contributor Author

I put #test in my commit message, but it seems like it's still skipping the test and release tests. Also, assuming all works correctly in the workflow file, this should be close to a finalized and working version

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 31, 2023

I think it should work now.

@coveralls
Copy link

coveralls commented Aug 31, 2023

Coverage Status

coverage: 95.212% (-0.4%) from 95.569% when pulling 00baae9 on wiboticanders:master into 1eefb91 on OpenCyphal:master.

@wiboticanders
Copy link
Contributor Author

wiboticanders commented Sep 1, 2023

Commit #[00baae9] revealed an error in the breakdown when the test fails. I was going through suggestions on the github site and not locally, and I thought it was all changes to the docs, so I committed the suggestions. One slipped past me that was a change to the interface return, which in turn broke the unit test because the returns did not match anymore. The problem stems from the way I collecting stderr and stdout after the yield, the communicate function never ended. So, that commit will not stop testing until whatever timeouts you have in place stop it. My apologies for overlooking that issue and pushing it, but I can't seem to stop it with my permissions so it has been running for a few hours.

In the previous commit it was mostly doc updates, but one changed the
socketcandmedia interface_name return. I forgot to update the unit test
assertions to match this, so in the CI testing it resulted in a
never ending loop. The issue stemmed from the way Popen gets stderr
and stdout with the communicate method, but only after the yield
statement. It does not respond to the kill() and instead runs forever,
so I replaced it with just a kill. Although you lose a little bit of
info, the socketcand instance gets checked before the unit test
is done in the fixture, and from there the daemon is very robust and
will not stop unless you force kill it or terminate it. So I think this
is the best solution, you still get output from errors within the unit
test as well.
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Please bump the minor version number in _version.py and drop a line in the changelog.

.github/workflows/test-and-release.yml Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Show resolved Hide resolved
pycyphal/transport/can/media/socketcand/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
tests/transport/can/media/_socketcand.py Outdated Show resolved Hide resolved
@wiboticanders
Copy link
Contributor Author

So new version should be 1.16.0?

Also made small tweaks based on code change requests,
removed some redundacy, slightly changed docs.
@pavel-kirienko pavel-kirienko merged commit 4846761 into OpenCyphal:master Sep 8, 2023
8 checks passed
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