Capital lettered service names are converted to small letters

[JasperTan97]

So I discovered a bug/feature (?) where capital letters of service names get made small. For example:

Service as defined in the component: publish_RAPID_var_pose
Service called by API: publish_RAPID_var_pose
Service gets called (202 response) but the service does not run.

Service as defined in the component: publish_RAPID_var_pose
Service called by API: publish_rapid_var_pose
Service gets called (202 response) and the service runs.

Furthermore, I found both capitalised and non-capitalised service names in ros2 service list.

[domire8]

Yes indeed, sorry that I didn't catch this immediately after a long day but the reason for this can be found here for cpp and here for python. In both cases we transform the string to lower case.

So this is not something from ROS but rather set by us historically. Of course this is not obvious, so there are several options:

1. Log a warning if the string was transformed
2. Allow all cases according to RCL
3. Just let ROS deal with it and catch errors
4. Leave as is and document it properly

What do you think @eeberhard @bpapaspyros

[JasperTan97]

was there a reason for this choice?

we should at least print warnings if we keep it in (changing my service names without letting me know was quite challenging to debug), and maybe write in the docs

[bpapaspyros]

huh, good to know, I hadn't noticed either. I like the RCL idea because this would potentially require less maintenance and let's say it's the ROS way of doing things, but at the same time I don't see a pressing reason to conform to this. Imo at this point perhaps option no 1 combined with adding it in the docs should be more than enough and doesn't break existing use cases, but this is a decision for the very core developers.

In fact, regardless of what we do it would be nice to have short mention for this in the docs.

[JasperTan97]

The other thing to note is that services that does not exist do not make a complain. The service simply gets called, and then the backend moves on. Is this intended? Or should we add a warning message?

[domire8]

The other thing to note is that services that does not exist do not make a complain.

Yeah that is a bit more complicated. As we can see here, the state engine makes a asynchronous request with the rclcpp Service client and we cant wait for the service to return something since that would block the rest of the state engine. Hence, we can't track an unsuccessful service call. Keeping a list of available services would be too much additional effort for the state engine.

[domire8]

For easier discussion, I will post the relevant lines in RCL here:

const char *
rcl_topic_name_validation_result_string(int validation_result)
{
  switch (validation_result) {
    case RCL_TOPIC_NAME_VALID:
      return NULL;
    case RCL_TOPIC_NAME_INVALID_IS_EMPTY_STRING: # We check for it explicitly
      return "topic name must not be empty string";
    case RCL_TOPIC_NAME_INVALID_ENDS_WITH_FORWARD_SLASH: # we remove slashes
      return "topic name must not end with a forward slash";
    case RCL_TOPIC_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS: # we remove unallowed characters
      return
        "topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}'";
    case RCL_TOPIC_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: # that, we don't check
      return "topic name token must not start with a number";
    case RCL_TOPIC_NAME_INVALID_UNMATCHED_CURLY_BRACE: # we don't allow braces in the first place
      return "topic name must not have unmatched (unbalanced) curly braces '{}'";
    case RCL_TOPIC_NAME_INVALID_MISPLACED_TILDE: # we don't allow tildes
      return "topic name must not have tilde '~' unless it is the first character";
    case RCL_TOPIC_NAME_INVALID_TILDE_NOT_FOLLOWED_BY_FORWARD_SLASH: # we don't allow slashes
      return "topic name must not have a tilde '~' that is not followed by a forward slash '/'";
    case RCL_TOPIC_NAME_INVALID_SUBSTITUTION_CONTAINS_UNALLOWED_CHARACTERS:
      return "substitution name must not contain characters other than alphanumerics or '_'";
    case RCL_TOPIC_NAME_INVALID_SUBSTITUTION_STARTS_WITH_NUMBER:
      return "substitution name must not start with a number";
    default:
      return "unknown result code for rcl topic name validation";
  }
}

As we can see, we can't actually get into most of these cases with our current sanitation of topic names (see my inline comments). There is only one case that we don't cover and it's when the string starts with a number. Hence, my suggestion is:

1. Prohibit strings that start with a number in modulo and treat it as feature and not a breaking change
2. Log a warn string if the provided topic name doesn't correspond to the sanitized one.
3. Put the fact that we do this kind of sanitation in the docs.

[domire8]

Sorry about double comments but I realized what exactly we are discussing here. Let's go step by step with the example of an input (outputs and services are treated the same):
We sanitize and use the signal_name parameter of add_input(const std::string& signal_name, const std::shared_ptr<DataT>& data) to decide on

1. The internal name of that input
2. The parameter name of the topic of that input (it will be <sanitized_signal_name>_topic)
3. The default topic name of that input (it will be ~/<sanitized_signal_name>)
4. The name of the input that a user will have to use in the YAML app (which is the same as point 2 since the state engine will take the name of the input under inputs, add _topic to it and set the parameter thats called <input_name>_topic)

What's important to note is that the actual topic name that we give to ROS when we create the subscriber is the value of the parameter which can be set by the user through the app, so it could be anything and making the subscriber might still fail even if we validated the signal name in the first place. I personally draw the following conclusions from this:

1. We are totally fine by being more restrictive than RCL with signal names since we give the possibility to the user to override the actual topic name.
2. It is even quite nice to be more restrictive than RCL since the YAML app looks cleaner if an input can only consist of lowercase letters, underscores and numbers
3. What we need to make sure is that the default topic name is a valid one, and that means we currently need to change the sanitation to prohibit strings that start with a number

In conclusion, I still propose the 3 steps from above and will add that if a user fails to set a valid topic name in the app, the creation of the subscribers through ROS will fail and modulo will correctly catch and log the error here

[bpapaspyros]

As we can see, we can't actually get into most of these cases with our current sanitation of topic names (see my inline comments). There is only one case that we don't cover and it's when the string starts with a number. Hence, my suggestion is:

1. Prohibit strings that start with a number in modulo and treat it as feature and not a breaking change
2. Log a warn string if the provided topic name doesn't correspond to the sanitized one.
3. Put the fact that we do this kind of sanitation in the docs.

Thank you for both explanations. I agree with the suggestions which make sense to me as a design.

[eeberhard]

Thank you for the clear write up, and sorry I could not give my feedback sooner. I completely agree that we prefer to be more restrictive because we are trying to maintain a consistent and opinionated abstraction in our application framework. We tend to use lower_snake_case everywhere. As a result, I also agree with @domire8's suggested implementation.

As a small note, our application schema restricts most namespaces to the regexp (^[a-zA-Z][a-zA-Z0-9_]*[a-zA-Z0-9]$)|(^[a-zA-Z]$), so it does not actually enforce lower case for things like named service calls in the YAML. Changing it to (^[a-z][a-z0-9_]*[a-z0-9]$)|(^[a-z]$) would be nice but would be a breaking change to the schema unfortunately (potentially invalidates previously valid schemas).

Still, we should suggest lower_snake_case in the docs and automatically sanitize to lower case in the implementation, and update the schema when the next breaking change happens.