Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

swolferDF
Copy link

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
  • ...

Sdpierret and others added 30 commits May 18, 2021 14:07
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]>
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]>
Signed-off-by: David Pierret <[email protected]>
Signed-off-by: David Pierret <[email protected]>
Signed-off-by: David Pierret <[email protected]>
Signed-off-by: David Pierret <[email protected]>
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
Sdpierret and others added 22 commits October 8, 2021 14:54
…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).
- 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 👌
Copy link
Member

@iguessthislldo iguessthislldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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

def accept(self, visitor):
visitor.visit_array(self)
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a reason to add pass?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +20 to +30
# 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"
# )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
# )

Comment on lines +14 to +16
PrimitiveType.Kind.byte: ('UByte', 'UByte(0x00)'),
PrimitiveType.Kind.u8: ('UByte', 'UByte(0x00)'),
PrimitiveType.Kind.i8: ('Byte', 'Byte(0x00)'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use int for uint8 and int8?

struct MySample2 {
@key string a_string;
double a_double;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iguessthislldo iguessthislldo changed the title Support qos Add Support for Publishing, More Types, and Basic QoS Nov 8, 2021
@iguessthislldo iguessthislldo added this to the v0.2 milestone Nov 8, 2021
@iguessthislldo iguessthislldo linked an issue Nov 8, 2021 that may be closed by this pull request
@iguessthislldo iguessthislldo marked this pull request as draft December 13, 2021 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array and sequence type not implemented
6 participants