Cleanup

[rhaschke]

Dear @guihomework, Trying to understand pwlf, I started first looking into your multi-calib commits. I have collected some required changes in this PR. Please have a look into the individual commits and the review comments I (will) add via github.

As far as I understand the basic idea was to allow different calibrations for different taxel subsets, which will be very useful. I have some suggestions regarding the semantics and yaml syntax to further improve:

• Why do you use the main key calib? Will there be any other key? My suggestion is to drop it and just use a sequence.
• The sensor_name is not used yet. Do we need it at all? Is that just for documentation?
• As far as I understand, single_calib_ is used for all taxels if:
  • there is no multi-calib given at all
  • there is no idx_range given for any sequence item
  • there is a single, negative idx specified in idx_range for any sequence item I suggest to drop this last condition or replace it by idx_range: default. My key suggestion is to use this as the default (or fallback) calib, if nothing more specific was defined for a taxel. Currently, single_calib_ is only actually used when calib_ is empty and calib_ is only used if it defines calibs for all taxel inputs. If we agree on this new semantics, I would cleanup the code some more towards this direction.

Hence, an example yaml config would look like this:

  - sensor_name: board0
    type: PWL
    idx_range: [[0, 50], [51,90]]
    values:
      0:    0
      1024: 10
      4095: 4095
  - sensor_name: board0
    type: PWL
    idx_range: default
    values:
      0:    -3.0
      4095: -5.0

What do you think?

[guihomework]

This PR is annoying as it might break something that is in production. it would be better to first discuss that

• Why do you use the main key calib? because I was asked to keep the code compatible with the old style for one caib-fo-all which was a yaml::map as is. Detecting the different types was easier with a starting tag.
• Will there be any other key? did not think about that. Currently not.

My suggestion is to drop it and just use a sequence

Dropping means changing the structure of the yaml which will break the code at our customers. This should be discussed if it is absolutely necessary now. And if yes, synchronize with them to do the switch.

Personallu I find the yaml easier to read if there is a top keyword, similar to the "" tag in all our xacros... although they are not robots.

• The sensor_name is not used yet. Do we need it at all? Is that just for documentation?

this is all because I had to plan ahead to provide code to customers but permit future extensions. We have incoming tactile_messages which contain several sensors with different names and different vector size. One must distinguish the calibration for those in the future. I would not say it is not "used" yet, I would say it is not "handled" yet, to have a 80/20 rule and get the basic function running, but avoid needing to change the calibration file again.

As far as I understand, singlecalib is used for all taxels if: there is no multi-calib given at all

yes if the old type of yaml calib is given (back ward compatibility with the code at our customer

there is no idx_range given for any sequence item

yes, this was the new default first idea, but was creating issues in some of the tools that did not handle so well empty sequences, so I think I wanted to switch to a negative value.

there is a single, negative idx specified in idx_range for any sequence item

I think I did not clarify this case yet.

I suggest to drop this last condition or replace it by idx_range: default.

The problem are that making this a string and not a vector of scala/vectors required to handle more switch cases and parsing than if it is always a vector. I see that it is could be easier for the human (but then more complex for the developer), but I believe some humans (me) prefer to have an empty vector or some specific value telling "all" than having a change of type.

My key suggestion is to use this as the default (or fallback) calib, if nothing more specific was defined for a taxel. Currently, singlecalib is only actually used when calib is empty and calib is only used if it defines calibs for all taxel inputs.

I need to process far more cases that we have in all our hardware to confirm what you say. I did it but now I doubt.

If we agree on this new semantics, I would cleanup the code some more towards this direction.

please no cleanup until we have a meeting.

[rhaschke]

The idea is to NOT require YAML, but I can see that I did not use #ifdefs everywhere. (one function needing a yaml node should be protected as well. This should be fixed, not requiring yaml

Essentially, the whole code doesn't work if yaml is not available, because file parsing requires yaml.

[rhaschke]

I would prefer to keep the verbose as it was.

Is this really needed by the customer or was that a debugging feature? As you said, introducing verbose was a strange choice, not following the standards. IMHO it makes the code uglier and unnecessarily complex. So, I still suggest to remove it. As a compromise, we could easily configure DEBUG level logging using the verbose ROS parameter. Yes, this still will drop the two-level verbosity.

[guihomework]

I have no idea how to respond to single answers to my inline comments, so it will be here

Then let's fix the code to work without YAML. It is made optional in tactile_filters, it must work here too. It is apparently not covered yet. My mistake, I know how this code was produced at first, in a rush due to bad time planning. so this was not done properly and I should fix it. This would be a new issue to report to make the code work without YAML.

[guihomework]

I would prefer to keep the verbose as it was.

Is this really needed by the customer or was that a debugging feature? As you said, introducing verbose was a strange choice, not following the standards. IMHO it makes the code uglier and unnecessarily complex. So, I still suggest to remove it. As a compromise, we could easily configure DEBUG level logging using the verbose ROS parameter. Yes, this still will drop the two-level verbosity.

apparently one can quote reply which copies things... ok.

The customer will not debug, I will debug with what the customer sends as error/log. So it must be simple for the customer to change verbosity and also not flood for basic debugging. If it is simple to change verbosity without making launch files uggly, maybe this a compromise

[rhaschke]

Dear @guihomework, I addressed the discussed issues and force-pushed to have a clean history. When doing the final review you might want to compare to the previous state (before the force-push). If your testing passes as well, this should be ready to merge. I left out the verbosity commit.

[rhaschke]

The log level cannot directly be set in a launch file, unfortunately. The only option is to provide a config file (in the package), which is configured via env: http://wiki.ros.org/rosconsole#Configuration Alternatively, we can set the log level easily in C++ code, based on the ROS parameter verbose: http://wiki.ros.org/rosconsole#Changing_Logger_Levels

Which method do you prefer?

[guihomework]

The log level cannot directly be set in a launch file, unfortunately. The only option is to provide a config file (in the package), which is configured via env: http://wiki.ros.org/rosconsole#Configuration Alternatively, we can set the log level easily in C++ code, based on the ROS parameter verbose: http://wiki.ros.org/rosconsole#Changing_Logger_Levels

Which method do you prefer?

Unfortunately you confirm what I was 90% sure of (as I checked myself not too long ago but might have missed something) I think only the first method (ENV) permits to set the log level for a named topic which would be a way to not lose the 2-level verbosity I introduced. If both permit to do it, I still prefer the first one, because one could have 3 config files, for each level of verbosity, then do a group if statement reading a verbose arg in the launch file, and load the appropriate config file. Seems like lot of overhead, but well, if we need to use ROS logging and not print outs, then we are stuck to this overhead.

[rhaschke]

I think only the first method (ENV) permits to set the log level for a named topic...

Both methods will work in principle. The first method kind of "teaches" users how to change log levels in general. The second method hides all these details from the user. So, my main question was: Do you want to teach the user or to simplify things for him?

[guihomework]

I think only the first method (ENV) permits to set the log level for a named topic...

Both methods will work in principle. The first method kind of "teaches" users how to change log levels in general. The second method hides all these details from the user. So, my main question was: Do you want to teach the user or to simplify things for him?

Either I did not understand the first method, or you did not intend to do a "if" in the launch that handles a verbose arg. For me both methods hide the exact loglevel settings. I don't want the user to need to edit and modify a file to get the debug, it should be possible from command line. A minimum of one level should be available this way (debug true or false) if verbose is too complex.

[rhaschke]

Log level names: ROS_PACKAGE_NAME.detail

[rhaschke]

@guihomework, I think I addressed the issues discussed offline. If you are fine now (and have approved), I will do some final history cleanup and merge.