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

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

Merged
merged 28 commits into from
Mar 16, 2021

Conversation

pavel-kirienko
Copy link
Member

@pavel-kirienko pavel-kirienko commented Feb 28, 2021

This is an implementation and a demonstration in support of OpenCyphal/public_regulated_data_types#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
Copy link

coveralls commented Feb 28, 2021

@davidlenfesty
Copy link

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
Copy link
Member Author

It was fixed about one month ago: OpenCyphal/pydsdl#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
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link
Member

thirtytwobits commented Mar 4, 2021

  • Provide a step-by-step set of instructions for setting up the proper directory structure for the demo.
  • Move the "Just-in-time vs. ahead-of-time DSDL compilation" section to after "running the application section"
  • start with a sentence or two about what the demo is demonstrating.
  • Print a message from the demo_app.py that says "running. ctrl+c to exit"
  • The section on Yakut; it's not clear that the examples are all to be run as separate processes. Perhaps you should be explicit about the number of shells. Something like "in addition to the terminal running pyuavcan open four more sessions and start the following yakut listeners..."
  • 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.
  • 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
  • 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.

@pavel-kirienko
Copy link
Member Author

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
Copy link
Member

thirtytwobits commented Mar 5, 2021

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
Copy link
Member

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
Copy link
Member Author

@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.

Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

SO much better now! Thanks Pavel.

…odule is not used by the library itself, it is needed only for testing the demo)
@pavel-kirienko pavel-kirienko merged commit 519d622 into master Mar 16, 2021
@pavel-kirienko pavel-kirienko deleted the env2 branch March 16, 2021 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants