-
Notifications
You must be signed in to change notification settings - Fork 60
Allow creating multiple channels #212
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
base: rolling
Are you sure you want to change the base?
Allow creating multiple channels #212
Conversation
Signed-off-by: Shravan Deva <devashravan7@gmail.com>
Signed-off-by: Shravan Deva <devashravan7@gmail.com>
|
@christophebedard Can you please review this? |
Signed-off-by: Shravan Deva <devashravan7@gmail.com>
Signed-off-by: Shravan Deva <devashravan7@gmail.com>
| events are enabled; if a list of lists is provided, one channel per list is created | ||
| with the corresponding UST events enabled in each channel | ||
| :param events_kernel: the list of kernel events to enable; if a list of lists is provided, | ||
| one channel per list is created with the corresponding kernel events enabled in each | ||
| channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotations for events_ust and events_kernel need to be updated; the typing-related test failures you see are probably related to this. However, it kind of highlights an issue I hadn't thought of, and I'm not quite sure how to solve it cleanly. I apologize for the long comment, but I think some context/explanation might be helpful.
Currently, events_ust is Optional[Iterable[SomeSubstitutionsType]]. It's kind of tricky, because SomeSubstitutionsType can be a single string/substitution, or it can be an iterable of strings/substitutions: https://github.com/ros2/launch/blob/1abf55ef6d3874ab5a4656e9ac0ea5d057c3c541/launch/launch/some_substitutions_type.py#L25-L30. If it's just a single string/substitution, then that's just 1 value, i.e., one event name. If it's an iterable, then it's still just 1 value: strings and substitutions (after being resolved) in the iterable will get concatenated into a single string. This is how substitutable inputs work.
Then, Iterable[SomeSubstitutionsType] means we have a list of values (where, again, each "value" can be a single string or an iterable of strings/substitutions that gets resolved and concatenated into a single value). normalize_to_list_of_substitutions() converts each SomeSubstitutionsType in that Iterable into a list of substitutions.
Since we want to support providing a list of this, we'd need something like List[Iterable[SomeSubstitutionsType]] (or Iterable[Iterable[SomeSubstitutionsType]]). The issue is that we don't know if it's a list of lists of individual values (single substitutions) or a list of lists that each represents a single value. Note that the output after normalization is definitely List[List[List[Substitution]]], though, like you did:
List[ # list of lists of events
List[ # list of events
List[Substitution] # single event (potentially made up of multiple substitutions that get resolved and concatenated into a single value, i.e., a single event name)
]
]To illustrate this, these two lists (before this PR) are functionally equivalent after normalization and substitution resolution:
events_ust_a = [
'ros2:*',
'ros2:rcl_publish',
'ros2:rclcpp_publish',
]
events_ust_b = [
'ros2:*', # or ['ros2:*']
['ros2:rcl_publish'],
['ros2:', 'rclcpp_publish'],
]With this PR, we'd like to do this:
events_ust_c = [
[ # one list of events for a channel
'ros2:*',
['ros2:rcl_publish'],
['ros2:', 'rclcpp_publish'],
],
[ # another list of events for another channel
'ros2:rmw_take',
'ros2:rcl_take',
],
]The problem is: How can you know if events_ust_c is a list of lists of events and not a list of lists of substitutions like events_ust_b? ['ros2:*', ['ros2:', 'rcl_publish']] is a single list of event names, but all(not isinstance(x, Iterable) or isinstance(x, (str, Generator)) for x in ['ros2:*', ['ros2:', 'rcl_publish']]) is False, meaning that the proposed code thinks it's actually a list of lists of events, when it's actually a single list of events.
An option might be to use sets to have separate lists of events, then you could do this:
if events_ust and not isinstance(events_ust, set):
# Single list
events_ust = [events_ust]
else:
# Multiple lists
events_ust = list(events_ust)However, there's a fundamental flaw with this. We currently accept an Iterable for events_ust, so a single "list" of events might actually be a single set of events, since a set is an Iterable. I'm not quite sure how to solve this in a clean and backwards-compatible way, but a 100% safe option would be to have a separate input for multiple (channel-specific) lists of events, e.g., events_ust_channels. If events_ust_channels is used (not None and not empty), then events_ust is ignored. This way, there is no doubt about whether the input is a list of lists of events or a single list of events. I don't like having a separate option, but since this is a more "advanced" feature, it might be acceptable.
What do you think? Did I miss anything?
Description
These changes will allow users to configure multiple channels in a tracing session.
This will create one channel with the UST events
ros2:rcl_initandros2:rcl_node_initenabled. Whereas if a list of lists is provided forevents_ust, example:2 channels are be created with UST events
ros2:rcl_init,ros2:rcl_node_initenabled in one channels andros2:rcl_initenabled in another channel.Fixes #199
Is this user-facing behavior change?
Did you use Generative AI?
Additional Information
This PR adds this functionality only to python launch files and not to xml files, yaml files, ros2trace cli, substitutions