Making SdoArray iterator behave equal to ODArray and *Record #430
Conversation
|
I'm not sure this is correct. PDO indexes are explicitly documented as starting with 1, not 0. I would assume the same being true for SDOs. Anyway, this is a breaking change; doc should be amended according to the new behaviour, iff this is an acceptable change. |
|
The iterator for PDOs; The only change here is if one iterates over |
Right, that was not immediately obvious to me; thanks for the clarification.
Well, it is still a behavioural change for code that iterates over |
|
The difference in behavior is definitely strange and I couldn't find an explanation why this was explicitly chosen for SDO only. Maybe @christiansandberg can clarify? Conceptually, I'd rather have the iterator be concerned with only the values in the SdoArray. Getting the count of items as the first entry seems unnatural for an array. We should think about making the behavior shown here also the default for the ODArray. But I do see a conceptual difference, as the OD describes the object entries and the count is one of those objects. But when accessing the values in the array via SDO, it makes sense to iterate only over the actual contents starting from subindex 1. Either way, we should make sure the |
|
With the change in this PR the following are observed:
ℹ️ NOTE: If this PR is not accepted, the
The CANopen standard supports this: "Sub-index 0 is of UNSIGNED8 and therefore not part of the ARRAY/RECORD data". (Paraphrased from 7.4.3 in CiA 301). So perhaps the iterator shoud omit index 0, however directly requesting it ( That said, in Python, it is unexpected behavior that a Mapping-like object returns an iterator doesn't iterate over all of its contents. |
|
One thing to take into consideration is that indices are dynamically sized. That's why you need to read the actual value of Last Subindex to know how many items there are. So that's why I didn't think it made sense to include Last Subindex as well as that is already included in the returned range. |
|
Maybe it would be more logic to exclude it in the records instead? It might make sense to exclude it from the Object Dictionary as well but it could be argued that Last Subindex is part of it and should be presented. |
|
Yes, it depends on which "hat" you're wearing: If you look at it from the object contents perspective, subindex 0 is superflous. If you look at what subobjects an array/record contains, then subindex 0 is definitely a part of the object. I start to think that the most logical solution would be that the iterators omit subindex 0, but it can be explicitly requested with While we're at it, perhaps |
In def __len__(self) -> int:
return self[0].rawIn def __len__(self) -> int:
return len(self.od)For completeness: I'm happy to change the
Yes. According to CANopen spec, subindex 0 is not a part of neither of them. |
|
Just a quick comment, need some more time to look at this thoroughly. I'm in favor of switching the len dunder to not do I/O and instead rely on the known OD structure. For one it's unexpected and I also noticed that will be a pain point / blocker for async. If one wants to know the content of subindex 0 explicitly, it's no trouble to request it via getitem. Skipping it while iterating sounds reasonable and if the spec is clear on that point for both records and arrays, let's follow that consistently as well. I'm still undecided on whether to remove it from OD iterators as well. That is a format description, where the element should definitely be noted. SDO on the other hand concerns the content, which does not include the length as the spec says. |
|
Iteration using SDO should probably be explicitly provided by method(s) to make it clear what you will get, rather than implicitly as today. Then it would be easier to provide an async equivalent. |
|
There is a suble detail here regarding arrays that we need to consider: Where is the authoritative information about an arrays length stored? Is it the OD or the actual device? # From SdoArray
def __iter__(self) -> Iterable[int]:
return iter(range(1, len(self)+1))
def __len__(self) -> int:
return len(self.od)Trying this on real devices, I quickly noticed that this relies on the OD keeping the count of the length of the array. However, the HW might have different length. So the iterator does not reflect the actual remote object. I need to read subindex 0 to get the actual length. So what should the iterator reflect here? Sidenote: If there are any nodes that doesn't update its subindex 0 to indicate array length, then the only solution is to iterate until a So for that use having the current code is more accurate, yet it opens up the problem of blocking IO in unexpected operations. def __len__(self) -> int:
return self[0].rawDo we need a similar mechanism as with PDO where you read the configuration into memory? I.e. for |
|
If I may add a comment with my async hat on in the hopes we can think a bit ahead: If we can avoid IO in |
|
You're certainly right in that we should treat the array length as dynamic, assuming that the OD does not contain the correct value. In fact, the OD might not even contain definitions for all array members. In that regard, iterating based on the actual subindex 0 count retrieved from the device seems correct. So maybe doing this based on Python's As a replacement, a separate iterator class would be a clean approach. There we can cache the length at an appropriate time, and have regular and async-friendly implementations side by side or within the same class. Or even keep the current interface for blocking and only an |
|
The following code is how its done in the async port. It presents a regular and async iterator. It does use the built-in dunders thou. So one cannot use a regular iterator within an async context. When the iterator relies on IO to get the length, I think the iterators will always need to be colored (regular and async) separate. def __iter__(self) -> Iterable[int]:
return iter(range(1, len(self) + 1))
async def aiter(self):
for i in range(1, await self.alen() + 1):
yield i
def __aiter__(self):
return self.aiter()
def __len__(self) -> int:
return self[0].raw
async def alen(self) -> int:
return await self[0].aget_raw() |
|
This discussion got a little bit derailed by mixing different topics: semantics, implementation characteristics (like unexpected blocking I/O), and async compatibility. I think we should focus on one at a time. Regarding semantics, I think we had a rough consensus:
Take for example an array where some members are reserved for future usage, skipping a subindex. These may not even show up in an EDS file, thus have no entries in our ODArray. The application hint in CiA 306, section 4.6.3.2 supports this. In theory, even subindex 0 could be missing from that. Thus iterating over an ODArray (or ODRecord) simply returns the OD knowledge, where subindex 0 could be present or not, as with any other subindex. Asking for "the length of the array" in the OD is not really meaningful, as we can get only the DefaultValue for subindex 0, which might differ from the actual length in the device.
For SDO, we're concerned with the actual data on the device. The number of entries (array length) should reflect the actual content of subindex 0. Whether cached or blocking I/O doesn't matter right now, it's an implementation detail. Iterating over an SdoArray should yield exactly as many entries as that subindex specifies (1...n), therefore excluding the count itself. Any entry undefined in the OD is generated on the fly by
For SdoRecord, it's a bit different (next asymmetry). The data types of sub-objects without an OD entry cannot be guessed, thus the only usable members are the ones we actually know from the OD. That defines the set of (and consequently, the number of) sub-objects usable with standard SDO functions. If the device reports higher subindices being supported (through the live subindex 0 count), or if there are gaps between known subs, those unknown entries could still be handled via
Summing up, I think this PR goes in the wrong direction, rather Sorry for the lengthy post. I'd be grateful for short assessments focusing on the six bullet points. Footnotes
|
|
To recap using the opening example, with #538 implemented, the current behavior can be summed up as: # Iterate over ODArray
a = [i for i in node.object_dictionary["MyArray"]] # Results in [0, 1, 2]
# Iterate over SdoArray
b = [i for i in node.sdo["MyArray"]] # Results in [1, 2]
# Iterate over ODRecord
a = [i for i in node.object_dictionary["MyRecord"]] # Results in [0, 1, 2]
# Iterate over SdoRecord
b = [i for i in node.sdo["MyRecord"]] # Results in [1, 2]This is consistent and logical. The need for this PR is no longer present and can be closed. |
Iterating over an
SdoArrayand iterating over anODArrayobject reveals different results.The
SdoRecordbehaves expectantly:This PR adds that the iterator for
SdoArrayinclude index 0. This is consistent with the iterator ofODArray,ODRecordandSdoRecord.