Add Support for Publishing, More Types, and Basic QoS #30

swolferDF · 2021-11-05T16:00:05Z

Finishing support Qos

Testing with PES good result
I changed :

update_redear_qos update_create_qos
Verify if listener is None in DataReader
_pyopendds.cpp
...

Add basic support for publisher know issue : Segfault after write() call Signed-off-by: David Pierret <[email protected]>

manage sequences as list

Signed-off-by: David Pierret <[email protected]>

Add basic support for publisher know issue : Segfault after write() call Signed-off-by: David Pierret <[email protected]>

manage sequences as list

Signed-off-by: David Pierret <[email protected]>

- Fix cpp issue where topic_types_ private member was accessed by derived class. - Fix DataWriter python class issue where the datawriter used datareader_wait_for cpp binding.

- DataWriter and Publisher classes should not have a listener member. - QoS still can be passed, but QOS_DEFAULTs are always used in cpp binding methods.

- Dash separation in label will be deprecated, use underscore instead

- DataWriter and DataReader wait_for methods does not use WaitSet dispatch. A simple while loop is used to check status conditions, and the STL current thread sleep is used to wait. - DataRead take_next_sample does not perform his own wait but try immediately perform library call to take_next_sample.

- When checking limits of floating value with cpp STL library, min() returns the minimum positive representable value, lowest() must be used instead.

- Each supported type defined in IDL imports now the whole module name at once (importing the root and browsing into submodules resulted into a NULL value and an exception).

- When a module has a type which is defined into a parent module, the type is not found. Imports are now done properly. - To investigate: Two Exceptions are now disabled in cpp bindings because the conditions seems not be required.

- DomainParticipant pointers were created with a factory, and then deleted with the help of SharedPointer (aliased to DomainParticipant_var). Mixing rough ptrs and shared ptrs causes troubles and the factory should now be responsible for the deletion of the participants.

- The IDL generated classes use Polymorphism (e.g: sequence type that directly derived from list). A simple call to Py_IsInstance() was not suffiscient and the exception was thrown (and unhandled) even though the case of the derived class should work.

- separate user.hpp and common.hpp into subfiles. - cpp/hpp files use now four spaces indentation

Signed-off-by: David Pierret <[email protected]>

…Supports # Conflicts: # pyopendds/dev/include/pyopendds/user.hpp

…Supports # Conflicts: # pyopendds/DataWriter.py # pyopendds/Publisher.py # pyopendds/dev/include/pyopendds/user.hpp # pyopendds/dev/itl2py/CppOutput.py # pyopendds/dev/itl2py/PythonOutput.py # pyopendds/dev/itl2py/ast.py # pyopendds/dev/itl2py/templates/user.cpp # pyopendds/dev/itl2py/templates/user.py # pyopendds/ext/_pyopendds.cpp # tests/basic_test/publisher.py

…Supports # Conflicts: # pyopendds/dev/include/pyopendds/user.hpp

(Reverse commit so that we mark the merging resolved as rejected).

Dev type supports

- pyidl supports multiple .idl files embed in one packages, related or not with include preprocessor directive

Verify if listener as None on DataReader add update datawriter Testing working well 👌

iguessthislldo

@Sdpierret @swolferDF I'd appreciate it if there were no more PRs that are based on commits from existing PRs. I know I've been very slow at getting back to reviewing, but creating new ones like this doesn't help me do this any faster. In the future please base new feature PRs off of the master branch if possible. I'm closing the other two and assuming this one is the one to review going forward.

This is a lot to go through, so in addition to the things I marked here are some general remarks:

Please revert the unnecessary style changes in the C++ code.
As part of this please remove and create a new PR for this pyidl command that is based on master since it doesn't seem related to QoS or any of the previous PRs. When you do this please also describe what the use cases for it are because I'm right now I'm still not sure what it's purpose is even after reading the description for it.
There are a number of TODO comments and commented out code added. Please resolve or remove them or add more details to the TODO comments if you don't think they're issues that have to be immediate addressed.
There are conflicting files, please ether rebase or merge in master to resolve the conflicts.
This isn't an issue now, but just to warn you the plan was and still is to generate QoS and other parts of the DDS API at least partially from the DDS IDL in OpenDDS. So the way the QoS works might be slightly different in the future.

After this is all done and the diff is smaller I will do a more detailed review.

iguessthislldo · 2021-11-08T03:18:30Z

pyopendds/dev/itl2py/ast.py

@@ -195,6 +195,7 @@ def __init__(self, base_type, dimensions):

    def accept(self, visitor):
        visitor.visit_array(self)
+        pass


Is this a reason to add pass?

iguessthislldo · 2021-11-08T03:18:49Z

pyopendds/dev/include/pyopendds/basictype.hpp

+template<typename T>
+class BooleanType {
+public:
+    static PyObject* get_python_class()


C++ code should follow OpenDDS' C++ style which has two space indents.

iguessthislldo · 2021-11-08T03:23:10Z

pyopendds/ext/_pyopendds.cpp

- *   Set default transport and discovery to RTPS unless default_rtps=False was
- *   passed.
- */
+* init_opendds_impl(*args[str], **kw) -> None


Please reverse all the places where this was done to multiline comments. There's no reason for this.

iguessthislldo · 2021-11-08T03:24:34Z

tests/basic_test/CMakeLists.txt

+# add_library(airbus_idl SHARED)
+# if(${CPP11_IDL})
+#     set(opendds_idl_mapping_option "-Lc++11")
+# endif()
+# OPENDDS_TARGET_SOURCES(airbus_idl "airbusdds.idl"
+#     OPENDDS_IDL_OPTIONS "-Gitl" "${opendds_idl_mapping_option}")
+# target_link_libraries(airbus_idl PUBLIC OpenDDS::Dcps)
+# export(
+#     TARGETS airbus_idl
+#     FILE "${CMAKE_CURRENT_BINARY_DIR}/airbus_idlConfig.cmake"
+# )


Remove?

Suggested change

# add_library(airbus_idl SHARED)

# if(${CPP11_IDL})

# set(opendds_idl_mapping_option "-Lc++11")

# endif()

# OPENDDS_TARGET_SOURCES(airbus_idl "airbusdds.idl"

# OPENDDS_IDL_OPTIONS "-Gitl" "${opendds_idl_mapping_option}")

# target_link_libraries(airbus_idl PUBLIC OpenDDS::Dcps)

# export(

# TARGETS airbus_idl

# FILE "${CMAKE_CURRENT_BINARY_DIR}/airbus_idlConfig.cmake"

# )

iguessthislldo · 2021-11-08T03:57:57Z

pyopendds/dev/itl2py/PythonOutput.py

+        PrimitiveType.Kind.byte: ('UByte', 'UByte(0x00)'),
+        PrimitiveType.Kind.u8: ('UByte', 'UByte(0x00)'),
+        PrimitiveType.Kind.i8: ('Byte', 'Byte(0x00)'),


Why not use int for uint8 and int8?

iguessthislldo · 2021-11-08T04:08:08Z

tests/typeSupport_test/TypeSupport_to_include.idl

+struct MySample2 {
+    @key string a_string;
+    double a_double;
+};


This file and a few others are missing the final newline at the end. Github shows this as a red marker at the end of the file.

Sdpierret and others added 30 commits May 18, 2021 14:07

Add publisher capability to pyopendds

6060c84

Add basic support for publisher know issue : Segfault after write() call Signed-off-by: David Pierret <[email protected]>

add Sequences support

46c2f41

manage sequences as list

add Floating type support

bb07ddc

Signed-off-by: David Pierret <[email protected]>

fix after review

5b99908

Signed-off-by: David Pierret <[email protected]>

Merge branch 'oci-labs:master' into master

e26b813

Add publisher capability to pyopendds

900c777

Add basic support for publisher know issue : Segfault after write() call Signed-off-by: David Pierret <[email protected]>

add Sequences support

da5df2e

manage sequences as list

fix after review

5773636

Signed-off-by: David Pierret <[email protected]>

add Floating type support

7feb5b2

Signed-off-by: David Pierret <[email protected]>

Add DataReaderListener for callback.

510b9a4

cleanup and more tests

6e5c98a

Signed-off-by: David Pierret <[email protected]>

add listener and fix publisher

8a50cad

Signed-off-by: David Pierret <[email protected]>

add boolean type support

6c67cb7

Signed-off-by: David Pierret <[email protected]>

fix wrong type in itl2py

cdffefa

Signed-off-by: David Pierret <[email protected]>

Add Char type to IntegerType

27cf722

Add more supported types

c9593f7

partial Qos addition to pyOpenDDS

7046ac4

Signed-off-by: David Pierret <[email protected]>

Merge branch 'dev_DataListener'

5bf68dc

Minor bug fix:

b5f84f3

- Fix cpp issue where topic_types_ private member was accessed by derived class. - Fix DataWriter python class issue where the datawriter used datareader_wait_for cpp binding.

Implementation improvements:

1244d6a

- DataWriter and Publisher classes should not have a listener member. - QoS still can be passed, but QOS_DEFAULTs are always used in cpp binding methods.

Fix setup.cfg warning:

39c7556

- Dash separation in label will be deprecated, use underscore instead

Fix an error where negative python floats in Samples causes crash:

b712b3e

- When checking limits of floating value with cpp STL library, min() returns the minimum positive representable value, lowest() must be used instead.

Add support for nested IDL modules:

3de4085

- Each supported type defined in IDL imports now the whole module name at once (importing the root and browsing into submodules resulted into a NULL value and an exception).

Minor code refactoring

573ffb9

Fix a bug for IDL nested modules:

1613d9f

- When a module has a type which is defined into a parent module, the type is not found. Imports are now done properly. - To investigate: Two Exceptions are now disabled in cpp bindings because the conditions seems not be required.

Fix warning and good practises python pep8 linter.

4e3b573

Refactoring:

a1dbfae

- separate user.hpp and common.hpp into subfiles. - cpp/hpp files use now four spaces indentation

Sdpierret and others added 22 commits October 8, 2021 14:54

add unsigned 8 and 32 bits integer

f9afdbb

Signed-off-by: David Pierret <[email protected]>

Merge commit 'f9afdbb52bebc574f816b4eb572b07b79454396f' into dev_Type…

2e84f03

…Supports # Conflicts: # pyopendds/dev/include/pyopendds/user.hpp

Merge commit 'bb07ddcf31390c8e6fcbafc7e2ddc24dff7105a6' into dev_Type…

668c88c

…Supports # Conflicts: # pyopendds/dev/include/pyopendds/user.hpp

Revert back merging from sequences branch.

b2b8971

(Reverse commit so that we mark the merging resolved as rejected).

Split long code lines in user.cpp with stringstream insertion.

6e63ad2

Merge branch 'master' into dev_TypeSupports

b68fac3

Merge pull request #1 from Sdpierret/dev_TypeSupports

3d35bcd

Dev type supports

Add pyidl console script:

c2b76c0

- pyidl supports multiple .idl files embed in one packages, related or not with include preprocessor directive

Lower cmake_minimum_required from 3.12 to 3.10

8327eb9

Run shell script for TypeSupport test

4af687d

Revert to cmake_minimum_required v3.12

496d3f8

Delete tata.idl

b6bb16d

Delete toto.idl

eeb23c1

qos dev try

3c74b40

p-e-s change config QOS

79e6342

Merge branch 'dev_PyidlConsoleScript' into support_QOS

f047637

modified function takenextsample() not blocking when subscribe

58ab265

dataReader qos Testing

79b43cd

dev qos datawriter (publisher)

1b0a5d9

Qos Working !! 🎉 🎉

1c80664

Verify if listener as None on DataReader add update datawriter Testing working well 👌

remove print

6578eac

iguessthislldo requested changes Nov 8, 2021

View reviewed changes

iguessthislldo changed the title ~~Support qos~~ Add Support for Publishing, More Types, and Basic QoS Nov 8, 2021

This was referenced Nov 8, 2021

Added sequences support on pyopendds [#18] #20

Closed

Add features to pyopendds #23

Closed

iguessthislldo added this to the v0.2 milestone Nov 8, 2021

iguessthislldo linked an issue Nov 8, 2021 that may be closed by this pull request

Array and sequence type not implemented #18

Open

add topicname optional arg callaback

df314df

iguessthislldo marked this pull request as draft December 13, 2021 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Publishing, More Types, and Basic QoS #30

Add Support for Publishing, More Types, and Basic QoS #30

swolferDF commented Nov 5, 2021

iguessthislldo left a comment

iguessthislldo Nov 8, 2021

iguessthislldo Nov 8, 2021

iguessthislldo Nov 8, 2021

iguessthislldo Nov 8, 2021

iguessthislldo Nov 8, 2021

iguessthislldo Nov 8, 2021

Add Support for Publishing, More Types, and Basic QoS #30

Are you sure you want to change the base?

Add Support for Publishing, More Types, and Basic QoS #30

Conversation

swolferDF commented Nov 5, 2021

Finishing support Qos

iguessthislldo left a comment

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment

iguessthislldo Nov 8, 2021

Choose a reason for hiding this comment