eclipse-cyclonedds / cyclonedds-python

Other
59 stars 47 forks source link

Python pub to C sub fails when using large/complex IDL #190

Open smnrgrs opened 1 year ago

smnrgrs commented 1 year ago

I've been struggling for a while with Python/C interoperability when using a complex topic (simpler ones work). On one Ubuntu 20.04 PC, Python to C interoperability fails (but succeeds if I comment out some of the topic fields). A clue is an OpenDDS subscriber says 'ERROR: Sedp::TypeLookupRequestReader::data_received_i - failed to take type lookup request'

I don't really know how to debug further so I've forked good old i11eperf and made branch 'complex-union' to demonstrate the problem: git clone https://github.com/smnrgrs/i11eperf.git and switch to branch 'complex-union'.

I have extended the 'ou' topic, and added a python publisher in the Python folder.

I see the following: Python pub -> osub FAIL Python pub -> asub FAIL apub -> osub OK Python pub -> Python sub OK

If the case TDTypeBd is removed from the IDL union at line 260, then interoperability works. Any suggestions would be gratefully received.

eboasson commented 1 year ago

I think https://github.com/eclipse-cyclonedds/cyclonedds-python/pull/191 does the trick, but I have only checked Python sub + apub and Python pub + asub.

smnrgrs commented 1 year ago

Thanks for looking at it so quickly, initial tests look promising, I'll update with something more definitive as soon as I can.

smnrgrs commented 1 year ago

Your change has certainly improved Python pub -> osub. There's possibly still some kind of issue with multi-line // comments in enums, I haven't quite figured it out yet.

smnrgrs commented 1 year ago

The changes in #191 certainly help @eboasson, but there are more problems. I've committed an update to the idl in https://github.com/smnrgrs/i11eperf.git which has more data in the union (and is closer to my full topic). With #191 in use the same test (python pub to asub/osub) fails, but works when an arbitrary change is made to the name of a data item (https://github.com/smnrgrs/i11eperf/blob/complex-union/idl/i11eperf.idl.works)

eboasson commented 1 year ago

Most curious!

Looking at python pub and asub, the funny thing is that this time the type identifiers (and hence type objects) are identical. Now it is the deserialisation of the data published by the python publisher that fails if the field is named dPValux and succeeds when it is named dPValue.

It does seem to make a difference if I change pub.py to use the correct field name. But surely it should never generate invalid CDR 🤔

(That's with https://github.com/eboasson/cyclonedds/tree/dynsub-typeobj-printer — I didn't check yet whether I might've added some change besides extending dynsub. I just wanted a quick look, so I skipped the homework!)

smnrgrs commented 1 year ago

Well I'm glad you can reproduce the failure!