Ensure the contents of the field are an array.

[clalancette]

It turns out that while all AbstractSequences are defined
to be arrays, we allow users to assign other sequences (like
lists) to those fields. Make sure that the field starts with
'array(' before trying to strip it off.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix ros2/build_farmer#212

[clalancette]

Here's a full CI run:

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

Please move the check into the else block since there is no reason for it on the if block.

[clalancette]

Done in 810ac05

[dirk-thomas]

When the user assigns a list to an array field I would expect the setter function to ensure that the actually stored value is an array.array: see

rosidl_python/rosidl_generator_py/resource/_msg.py.em

Lines 520 to 524 in f78c207

	@[ if isinstance(member.type, Array)]@
	self._@(member.name) = numpy.array(value, dtype=@(SPECIAL_NESTED_BASIC_TYPES[member.type.value_type.typename]['dtype']))
	@[ elif isinstance(member.type, AbstractSequence)]@
	self._@(member.name) = array.array('@(SPECIAL_NESTED_BASIC_TYPES[member.type.value_type.typename]['type_code'])', value)
	@[ end if]@

So I think before making this change we need to investigate why that is not the case (and maybe address that instead).

[clalancette]

So I think before making this change we need to investigate why that is not the case (and maybe address that instead).

Yeah, that occurred to me too. I put this up so we could get it in quickly, but since this isn't a rush for release now we should probably investigate that.

[clalancette]

So I think before making this change we need to investigate why that is not the case (and maybe address that instead).

I see where the discrepancy is. The problem shows up for types that are not in the SPECIAL_NESTED_BASIC_TYPES. If the message has a Sequence of booleans, for instance, there is no way to directly map them to an array. So the generated setter only checks if the new value is a valid Sequence, and does a direct assignment. While we could potentially think about coercion to an array for some of the primitive types, this wouldn't work in general for Sequences of non-primitives.

I think the fallout from this is that we do need to handle regular Sequences along with array types, and hence this patch still makes sense. Thoughts?

[dirk-thomas]

Based on your argument I don't think the current patch is right. I would rather change the condition isinstance(t, rosidl_parser.definition.AbstractSequence) to also check for SPECIAL_NESTED_BASIC_TYPES.

[clalancette]

`` to also check for SPECIAL_NESTED_BASIC_TYPES.

The problem with that is that SPECIAL_NESTED_BASIC_TYPES comes from https://github.com/ros2/rosidl_python/blob/master/rosidl_generator_py/rosidl_generator_py/generate_py_impl.py#L37 , and we don't currently import rosidl_generator_py at runtime for the generated messages. Our options seem to be:

1. Import rosidl_generator_py at runtime
   1. Do it at the top of the file
   2. Do it just in repr (slightly better performance if repr is never called)
2. Move SPECIAL_NESTED_BASIC_TYPES to rosidl_parser.definition (I'm not sure what additional changes would be required with that)
3. Keep with what this patch does and use an indirect check (presence of 'array(') to determine this

I'm not really hugely in favor of importing rosidl_generator_py at runtime, mostly because we slow down all message code with an additional import. However, I could be convinced otherwise.

[dirk-thomas]

How about 4. use SPECIAL_NESTED_BASIC_TYPES in the template and expand the actual values into the generated code?

[clalancette]

How about 4. use SPECIAL_NESTED_BASIC_TYPES in the template and expand the actual values into the generated code?

Done in 0e8e345. New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

Nitpick: the multi line condition isn't distinguishable from the indented block. Please use the following style instead:

if (
    ... and
    ...
):
    ...

[clalancette]

Done in f7b18f6

[clalancette]

CI is green (Windows test failures are unrelated), so I'm going to merge.