search

OpenCyphal / pycyphal

Python implementation of the Cyphal protocol stack.
https://pycyphal.readthedocs.io/
MIT License
119 stars 106 forks source link

Extend the application-layer API to avoid hard-coded protocol-related constants in applications #154

Closed pavel-kirienko closed 3 years ago

pavel-kirienko commented 3 years ago

This is an implementation and a demonstration in support of https://github.com/UAVCAN/public_regulated_data_types/pull/109.

Instead of specifying the port-ID explicitly when instantiating a port: node.presentation.make_publisher(Type, 1234) Now, one uses indirection to source the port-ID from the registry: node.make_publisher(Type, 'my_subject') The new node factory make_node() removes boilerplate and hard-coded identifiers.

Please find the documentation for this change on the readthedocs PR build site:

It would be super if one could follow the tutorial along to make sure that it doesn't omit any crucial details. If you do that, note that you will need to install Yakut from a branch for now (I will merge/release it immediately after this PR is accepted):

pip install git+https://github.com/UAVCAN/yakut@orc

I do not recommend reviewing the code because our limited resources are better spent on more important things.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 38240182

Files with Coverage Reduction New Missed Lines %
/home/appveyor/projects/pyuavcan/pyuavcan/presentation/_port/_subscriber.py 1 82.76%
/home/appveyor/projects/pyuavcan/pyuavcan/application/heartbeat_publisher.py 2 96.4%
/home/appveyor/projects/pyuavcan/pyuavcan/dsdl/_builtin_form.py 2 97.2%
/home/appveyor/projects/pyuavcan/pyuavcan/transport/serial/_serial.py 2 92.73%
/home/appveyor/projects/pyuavcan/pyuavcan/application/diagnostic.py 3 93.02%
/home/appveyor/projects/pyuavcan/pyuavcan/application/_node.py 3 96.85%
/home/appveyor/projects/pyuavcan/pyuavcan/dsdl/_compiler.py 4 93.22%
/home/appveyor/projects/pyuavcan/pyuavcan/presentation/_port/_publisher.py 4 90.0%
/home/appveyor/projects/pyuavcan/tests/demo/_demo_app.py 7 97.12%
/home/appveyor/projects/pyuavcan/pyuavcan/application/plug_and_play.py 13 92.24%
<!-- Total: 75 -->
Totals Coverage Status
Change from base Build 37726864: 0.3%
Covered Lines: 12864
Relevant Lines: 13294
💛 - Coveralls
davidlenfesty commented 3 years ago

I was trying to work through this and found a bug with the DSDL generation:

pydsdl._error.InternalError: /tmp/pyuavcan_demo/public_regulated_data_types/uavcan/diagnostic/8184.Record.1.1.uavcan:4: Conflicting definitions: [DSDLDefinition(full_name='uavcan.time.SynchronizedTimestamp', version=Version(major=1, minor=0), fixed_port_id=None, file_path='/tmp/pyuavcan_demo/public_regulated_data_types/uavcan/time/SynchronizedTimestamp.1.0.uavcan'), DSDLDefinition(full_name='uavcan.time.SynchronizedTimestamp', version=Version(major=1, minor=0), fixed_port_id=None, file_path='/tmp/pyuavcan_demo/public_regulated_data_types/uavcan/time/SynchronizedTimestamp.1.0.uavcan')]

I get this when running yakut compile public_regulated_data/uavcan on the master branch with the version of yakut you specified (not when running it with 0.1.5). It seems to come from this branch of pyuavcan but I haven't fully verified that.

pavel-kirienko commented 3 years ago

It was fixed about one month ago: https://github.com/UAVCAN/pydsdl/pull/61

Please make sure you are running the latest PyDSDL: pip install -U pydsdl. I guess we should bump the minimal required version in Nunavut.

davidlenfesty commented 3 years ago

Okay with updated dependancies (python package management is just a joy...) I got everything to run well, except for orchestration but I'm assuming that's not important to this PR, and don't have too much time to play with this.

One nit is that the demo app's filename should probably be specified immediately prior to the code, the snippet given in "Running the Application" is the first reference to the filename used for the demo.

pavel-kirienko commented 3 years ago

I got everything to run well, except for orchestration but I'm assuming that's not important to this PR

The directory removal problem should be fixed now but you are right, this is not that important.

Do you think such register integration can be implemented in uavcan.rs?

davidlenfesty commented 3 years ago

Do you think such register integration can be implemented in uavcan.rs?

Yep, although I don't plan on implementing it where I am currently working (which is roughly equivalent to the presentation layer in pyuavcan). I want the core uavcan-rs crate to be as general and cross-platform as possible.

A user-friendly "application" crate on top of the core crate would likely be what I would want to work on after I hit 1.0, and register integration is definitely a natural feature to add there.

pavel-kirienko commented 3 years ago

A user-friendly "application" crate on top of the core crate would likely be what I would want to work on after I hit 1.0, and register integration is definitely a natural feature to add there.

Okay. I think it's critical though to make the application crate no-std as well. We don't want embedded developers to reinvent this wheel.

thirtytwobits commented 3 years ago
pavel-kirienko commented 3 years ago

Startup sometimes throws:

It is supposed to do that when you run the application for the first time (i.e., the database file is not yet created) without exporting environment variables. Can you confirm that it happens after you export the variables?

Can we make this into a config file that is read into the demo app?

Kind of like this one? https://github.com/pavel-kirienko/yukon/blob/master/test.orc.yaml

In a way, that's what the last part of the tutorial is about.

thirtytwobits commented 3 years ago

I'd comment out the line that requires ncat and change the comment to "if you have ncat you can uncomment this line and..."

(.penv) $ yakut orc launch.orc.yaml
/bin/sh: 1: ncat: not found
thirtytwobits commented 3 years ago

Can you confirm that it happens after you export the variables?

Yeah. The ports and the IP coming from a config file would make this demo so much better.

pavel-kirienko commented 3 years ago

@thirtytwobits check out my latest changes. I think I addressed everything except:

the environment variables for the port identifiers is clunky. Can we make this into a config file that is read into the demo app? This also makes it WAY easier to debug.

This is not really meant to be very human-friendly. I think it makes sense to start with the basics even if they are not particularly convenient and then gradually introduce more advanced matters like the orchestrator. The orchestration file is essentially the config file you are looking for, but if we introduce it from the start it may get confusing.

Log a warning from demo_app.py if the file is not called demo_app.py since the orc-file part of the tutorial will fail with a big ol' stack trace if you don't adhere to this convention.

I addressed this by making the demo easier to follow along step-by-step such that by the time the reader gets to the orchestrator the file should already be created. I also added another hint about this right after the orchestration script for good measure.

yakut pub --count 10 2345:uavcan.si.unit.temperature.Scalar.1.0 'kelvin: 250' ... example doesn't work (nothing is printed by yakut sub -M 2347:uavcan.si.unit.voltage.Scalar.1.0

Not reproducible, neither locally nor in CI.