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

Inclomplete Configuration Examples and Missing Documentation #25

Open
YBachmann opened this issue Nov 27, 2024 · 9 comments
Open

Inclomplete Configuration Examples and Missing Documentation #25

YBachmann opened this issue Nov 27, 2024 · 9 comments
Assignees

Comments

@YBachmann
Copy link

I noticed some discrepancies in the provided configuration files.

The file /config/ods_default_parameters.yaml includes a line config_file: "config/examples/o3r_ods.json" but config/examples/o3r_ods.json does not exist in the repository.

The other yaml files in the config folder (o3r_2d.yaml, o3r_3d.yaml, two_o3r_heads.yaml and camera_default_parameters.yaml) do not include the config_file parameter at all.

This confused me at first because there is no direct hint or documentation that the yaml configuration files need a reference to another json configuration file.

Also by not providing a default configuration file users will get inconsistent behaviour when running the same launch command multiple times because not a fixed default configuration will be loaded but rather the last-saved settings from the VPU (which can get modified in different places like the vision assistant).

Proposed changes:

  • add the config_file parameter in all yaml configuration files (e.g. config_file: "config/examples/o3r_port_settings.json")
  • create default json configuration files (e.g. config/examples/o3r_port_settings.json and config/examples/o3r_ods.json)
  • explain in the documentation that there are two kind of configuration files. For the nodes and their ros specific parameters there are yaml files. For the o3r specific parameters, there are json files that get referenced in the yaml files.

What do you think of this? I can happily create a PR to implement the proposed changes.

@lola-masson
Copy link
Member

Hi @YBachmann,

Thanks a lot for the suggestion! We would happily review a PR with these changes.

One thing to keep in mind, is that there will be a "duplicate" of port info, in the yaml (pcic_port), and in the json (/ports/portX). So this should be clear in the documentation: if you accidentally provide a json file with a port number that differs from the one in the yaml, then the configuration will be done "for nothing", if that makes sense.

@YBachmann
Copy link
Author

YBachmann commented Dec 12, 2024

Hey @lola-masson ,

Is there a reason why the pcic_port is is defined at two separate places? I noticed that the yaml config files also contains other duplicate parameters such as ip (ipv4 in the json config) or buffer_id_list (availablePCICOutput in the json config).

Also, I tried removing components from the json config that are unnecessary for an example file (device/clock, device/diagnostic, device/swVersion, ports/portX/info/100007084421, ...) but if I do so I get an error like this: [camera_standalone-1] 2024-12-12 14:37:25 ERROR [../modules/device/src/libifm3d_device/xmlrpc_wrapper.hpp:100] http://172.24.2.25:80/api/rpc/v1/com.ifm.efector/ -> set: RPC response indicates failure. : validation failed for additional property 'processing': instance invalid as per false-schema; instance: .....
Is it necessary to always use a complete json-config? In that case adding an example file would probably not make much sense as it would have to contain a lot of unnecessary or wrong (e.g. serialNumber) data.

@YBachmann
Copy link
Author

Hi @lola-masson ,
just checking in to see if there are any updates regarding my previous comment.

@lola-masson
Copy link
Member

Hi @YBachmann,
So sorry for the delay answering your requests, I was away for a while. I am now back and will take some time to look through your questions and PR shortly.

@lola-masson lola-masson self-assigned this Feb 5, 2025
@lola-masson
Copy link
Member

Regarding the JSON structure, you do not need to use the complete JSON file, you can use only a subset. Considering the error message that you shared, my best guess is that there is an error in the JSON file you are trying to use. You need to preserve the hierarchy of the JSON, even if you remove some elements. So if you are changing a processing value, the file would look something like (replace with your actual config):

{
"ports": { 
  "port0": {
    "processing": {
      "maxDistNoise": 0.1
      }
    }
  }
}

@YBachmann
Copy link
Author

You are right, it seems I made some sort of mistake earlier.
I tried again and was able to remove everything that should not be included in an example file (serial numbers, current time and temperatures, ...).

The only error I encountered was

[ods_standalone-1] [ERROR] [1738765762.601717592] [ifm3d.vpu_a.ods]: Caught exception in callback for transition 10
[ods_standalone-1] [ERROR] [1738765762.601746188] [ifm3d.vpu_a.ods]: Original error: Unknown error 104011: /calibration: required property 'version' not found in object; instance: {}

after I removed the "version": { ... } component from "ports": { "port0": { "info": { "calibration": { .... }.
Removing the complete "calibration" component solved this.

"info": {
    "calibration": {
        "version": {
            "major": 0,
            "minor": 1,
            "patch": 1
        }
    },

The only remaining question I have is regarding the duplicates in the ros parameter yaml file and the .json/.o3rcfg config_file:

Those are the ones I noticed:

ros parameter yaml file ifm config json file
pcic_port pcicTCPPort
ip ipv4
buffer_id_list availablePCICOutput

@lola-masson
Copy link
Member

There are not exactly duplicate in the sense that they are not used the same way in the ROS node.
The configuration file will configure the device upon start of the node, for example specific filters, etc. This is a configuration for the whole device, so you could also put there network settings, application settings, etc. The ROS node will use the complete configuration file and not only the part of the config that relates to the port that the ROS node uses. A configuration file also could contain config for multiple ports. This means that we need to have a way to tell the ROS node which port or app it should take care of. This is what the pcic_port parameter in the yaml file is used for.
For the IP, you are right that there is only one and we could read this in the config file. This would require adding some logic in the ROS node to analyze the provided config file to extract the IP address if it exists. We need an IP address to use any of the ifm3d API tools, so for simplicity we require it from the user so we can instantiate all the relevant objects and use the API right away.
For the buffer_id_list, the one configured in the yaml file defines the data streams that the user wants to receive. The availablePCICOutput just lists what comes out of the device. The user could decide to ignore some of the data streams, in which case the corresponding publishers will never be instantiated in the ROS node. Note that the availablePCICOutput does not list everything that is available, because some buffers are constructed directly by the API (this is the case for the XYZ buffer for example).

@YBachmann
Copy link
Author

Alright, thank you very much for the explanation 🙂

I will add a minimal default config_file in PR #27

Just one more small question: The config_file can have have both a .json or a .o3rcfg extension, right?

@lola-masson
Copy link
Member

Just one more small question: The config_file can have have both a .json or a .o3rcfg extension, right?

Good point. I guess it is possible, we do not check for file extension in the ROS node, only that it can be parsed as JSON.

Thank you for the work in the PR!

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

No branches or pull requests

2 participants