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

GSoC - Adds Qserial protocol into Communication Plugin #20

Merged

Conversation

techytoes
Copy link

This PR adds Qserial protocol into the communication plugin as part of GSOC project. Please review it @ErwanDouaille


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

find_package(Qt5Multimedia REQUIRED)
find_package(Qt5Widgets REQUIRED)
find_package(Qt5Gui REQUIRED)
find_package(Qt5SerialPort)

Choose a reason for hiding this comment

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

this one is required. But why you need gui, widget, multimedia ... ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, others packages are not required. I will remove them in further commits.

add_subdirectory(Communication_test)

IF(WIN32)
add_definitions(-D_WINSOCKAPI_)
ENDIF(WIN32)

add_library(${PROJECT_NAME} SHARED ${HEADER_FILES} ${SOURCE_FILES} ${EXTRA_FILES})
target_link_libraries(${PROJECT_NAME} ${Oscpack_LIBRARIES} ${ZMQ_LIBRARIES} ${VRPN_LIBRARIES} SofaCore SofaSimulationGraph)
target_link_libraries(${PROJECT_NAME} ${Oscpack_LIBRARIES} ${ZMQ_LIBRARIES} ${VRPN_LIBRARIES} SofaCore SofaSimulationGraph Qt5::Widgets)

Choose a reason for hiding this comment

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

widget ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this too. I will remove it.

#### Receive

```
<ServerCommunicationQSerial name="qserial" job="receiver" port="6000"/>

Choose a reason for hiding this comment

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

port ?

Copy link
Author

Choose a reason for hiding this comment

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

So, should I hardcode the port name into the code?

Choose a reason for hiding this comment

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

what is port for serial communication ?

Copy link
Author

Choose a reason for hiding this comment

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

Its something like /dev/ttyUSB or /dev/ttyACM

serial->setPort(port);
serial->setBaudRate(QSerialPort::Baud9600);
serial->setDataBits(QSerialPort::Data8);
if(serial->open(QIODevice::ReadWrite))

Choose a reason for hiding this comment

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

write should not be there, open once then loop on subscribers

serial->setDataBits(QSerialPort::Data8);
if (serial->open(QIODevice::ReadOnly))
{
serial->setDataTerminalReady(true);

Choose a reason for hiding this comment

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

open it only one time then loop on it.

Copy link
Author

Choose a reason for hiding this comment

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

Can we skip this part because I will not be able to test it, even if I make suitable changes?

Choose a reason for hiding this comment

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

you are not able to test it but it please, do the changes :)

std::string port = d_port.getValueString();
while(m_running)
{
serial->setPort(port);

Choose a reason for hiding this comment

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

port ? AFAIK no port needed :/
What is it ?

@ErwanDouaille
Copy link

Don't worry. I will look at it the next week. I'm overbooked :(

@techytoes
Copy link
Author

@ErwanDouaille No problem. It is fine. Even, I am very busy right now in college stuff.

@marques-bruno marques-bruno merged commit 4015575 into SofaDefrost:sofaCommunication Jul 29, 2020
guparan added a commit that referenced this pull request Jul 31, 2020
This reverts commit 4015575, reversing
changes made to 8b28d31.
@guparan
Copy link
Collaborator

guparan commented Jul 31, 2020

I had to revert this PR since it is unfinished work, not ready at all.

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.

None yet

4 participants