PDO getitem unexpected returns `PdoVariable` on value 0

[sveinse]

Reading PDO maps from a node is a 1-indexed value. However, if the user use index 0 (which is a common expectation for many indexes), __getitem__ returns a PdoVariable instead of the expected PdoMap.

node = network.create_node(42, "mynode.eds")
node.rpdo.aread(from_od=False)

# Use correct 1 index
rpdo = node.rpdo[1]    # Type returned is `PdoMap`

# Use incorrect 0 index
rpdo = node.rpdo[0]    # Type returned is a `PdoVariable`

The source is due to the else in this code

canopen/canopen/pdo/base.py

Lines 42 to 54 in e840449

	def __getitem__(self, key):
	if isinstance(key, int) and (0x1A00 <= key <= 0x1BFF or # By TPDO ID (512)
	0x1600 <= key <= 0x17FF or # By RPDO ID (512)
	0 < key <= 512): # By PDO Index
	return self.map[key]
	else:
	for pdo_map in self.map.values():
	try:
	return pdo_map[key]
	except KeyError:
	# ignore if one specific PDO does not have the key and try the next one
	continue
	raise KeyError(f"PDO: {key} was not found in any map")

We just spent an hour trying to debug why the PDO didn't work before we realized the obvious mistake. I think using 0 index is such a common expectation, that I think we should give an error or clearly warn the user.

What is the intent of the the else section in L48-L53? When is this used? I could not find any tests showing how this is used. Because of the else rpdo[0] as shown above will fetch the first PdoVariable from the first PDO, but rpdo[1] will not fetch the second PdoVariable.

[acolomb]

It's a mechanism to search through all PDO mappings for a certain OD index entry, as I understand it. So you can access node.rpdo[0x6060] for example, without knowing which specific PDO has that object mapped. It should work even with strings as key.

Passing the special value 0 down to that mechanism doesn't make sense at all. Of course a PdoMap is rightfully indexable from 0 to 7. But of those possible values, 0 is the only one which could be passed down from PdoBase.__getitem__(). You are absolutely right that this is very error-prone and badly designed. I think we should explicitly filter out the 0 value in the else branch, as it is also left out from the previous branch and ends up there accidentally.

[sveinse]

I have tested it further and I find it somewhat inconsistent. See the examples below.

The function returns PdoVariable for many input values. To be able to use it, e.g. to do PDO transmit, one can use .pdo_parent to access its PdoMap. However any object can be member of multiple PDOs, but this function will only be able find the first occurrence of the PDO variable.

Using int argument with an index to an array or record object will make the function return the first PdoVariable which is used in the PDOs. Its not possible to retrieve any other subindex objects this way.

Lastly, the code seems to suggest that using the COB-ID for the PDO mapping, e.g. 0x1600 should return the PdoMap for RPDO1, but this doesn't work.

# Returns the first PDO mapping (expected)
print(node.rpdo[1])  # type: PdoMap

# Returns the first PDO index in the RPDO list (unexpected)
print(node.rpdo[0])  # type: PdoVariable

# RPDO1. Fails with a KeyError, even if code seems to suggest it to be correct.
print(node.rpdo[0x1600])

# 0x2015 is a record object. This returns the first occurrence of any
# subindex present in the PDO. In this case, it returns 0x2015.2
# as it happens to be the first subindex used in the PDOs.
print(node.rpdo[0x2015])  # type: PdoVariable

# Get the object by name.
print(node.rpdo["Breathing.Respiratory rate"])  # type: PdoVariable

# Getting an object that doesn't exist in any PDO fails with KeyError (expected)
print(node.rpdo["Inrush delay"])

[acolomb]

Alright, this is pretty convoluted. The access based on mapping parameter index (0x1600, 0x1A00) only works on the node.pdo: PdoBase object. It replaces its dedicated pdo.map: Optional[PdoMaps] attribute with a plain dictionary, adding only the index-based lookups. That even makes kind of sense (except for type mangling) because node.pdo[2] would be ambiguous whether we want RX or TX.

On the other hand, node.rpdo: PdoBase and node.tpdo: PdoBase do use the PdoMaps wrapper, and its constructor generates the lookup for PdoMap only based on the relative index number (1 to 512, filtering out those where no communication parameter record is in the OD). This means that node.rpdo[0x1600] will NOT work, which is a bit unexpected, see #607 @maxwai.

I intend to unify this in the linked PR #609. Just noting here for future reference where the confusion stems from.