-
Notifications
You must be signed in to change notification settings - Fork 177
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
new internal event functions with ownership handling (ownership used for linux implementation only) #1222
new internal event functions with ownership handling (ownership used for linux implementation only) #1222
Changes from 7 commits
6687670
850cbf3
6e47bb9
e597793
cfc5231
e73e464
85e193e
b83511b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* ========================= eCAL LICENSE ================================= | ||
* | ||
* Copyright (C) 2016 - 2019 Continental Corporation | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
* ========================= eCAL LICENSE ================================= | ||
*/ | ||
|
||
/** | ||
* @file ecal_event_internal.h | ||
* @brief eCAL event interface (internal) | ||
* | ||
* This file will be renamed back to ecal_event.h after removing event API from eCAL's public API. | ||
**/ | ||
|
||
#pragma once | ||
|
||
#include <ecal/ecal_eventhandle.h> | ||
|
||
#include <string> | ||
|
||
namespace eCAL | ||
{ | ||
/** | ||
* @brief Open a named event with ownership. | ||
* | ||
* @param [out] event_ Returned event struct. | ||
* @param event_name_ Event name. | ||
* @param ownership_ Event is owned by the caller and will be destroyed on CloseEvent | ||
* | ||
* @return True if succeeded. | ||
**/ | ||
bool gOpenNamedEvent(eCAL::EventHandleT* event_, const std::string& event_name_, bool ownership_); | ||
|
||
/** | ||
* @brief Open an unnamed event. | ||
* | ||
* @param [out] event_ Returned event struct. | ||
* | ||
* @return True if succeeded. | ||
**/ | ||
bool gOpenUnnamedEvent(eCAL::EventHandleT* event_); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -869,10 +869,6 @@ TEST(IO, MultipleSendsUDP) | |
eCAL::Finalize(); | ||
} | ||
|
||
|
||
|
||
|
||
|
||
#if 0 | ||
TEST(IO, ZeroPayloadMessageTCP) | ||
{ | ||
|
@@ -923,8 +919,6 @@ TEST(IO, ZeroPayloadMessageTCP) | |
} | ||
#endif | ||
|
||
#include <ecal/msg/string/publisher.h> | ||
#include <ecal/msg/string/subscriber.h> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These includes should be needed but maybe we can move them to the top? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They are there and have been there before .. |
||
TEST(IO, DestroyInCallback) | ||
{ | ||
/* Test setup : | ||
|
@@ -988,4 +982,75 @@ TEST(IO, DestroyInCallback) | |
// finalize eCAL API | ||
// without destroying any pub / sub | ||
eCAL::Finalize(); | ||
} | ||
} | ||
|
||
TEST(IO, SubscriberReconnection) | ||
{ | ||
/* Test setup : | ||
* publisher runs permanently in a thread | ||
* subscriber start reading | ||
* subscriber gets out of scope (destruction) | ||
* subscriber starts again in a new scope | ||
* Test ensures that subscriber is reconnecting and all sync mechanism are working properly again. | ||
*/ | ||
|
||
// initialize eCAL API | ||
eCAL::Initialize(0, nullptr, "SubscriberReconnection"); | ||
|
||
// enable loop back communication in the same thread | ||
eCAL::Util::EnableLoopback(true); | ||
|
||
// start publishing thread | ||
std::atomic<bool> stop_publishing(false); | ||
eCAL::string::CPublisher<std::string> pub_foo("foo"); | ||
std::thread pub_foo_t([&pub_foo, &stop_publishing]() { | ||
while (!stop_publishing) | ||
{ | ||
pub_foo.Send("Hello World"); | ||
std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
} | ||
std::cout << "Stopped publishing" << std::endl; | ||
}); | ||
|
||
// scope 1 | ||
{ | ||
size_t callback_received_count(0); | ||
|
||
eCAL::string::CSubscriber<std::string> sub_foo("foo"); | ||
auto receive_lambda = [&sub_foo, &callback_received_count](const char* /*topic_*/, const std::string& /*msg*/, long long /*time_*/, long long /*clock_*/, long long /*id_*/) { | ||
std::cout << "Receiving in scope 1" << std::endl; | ||
callback_received_count++; | ||
}; | ||
sub_foo.AddReceiveCallback(receive_lambda); | ||
|
||
// sleep for 2 seconds, we should receive something | ||
std::this_thread::sleep_for(std::chrono::seconds(2)); | ||
|
||
EXPECT_TRUE(callback_received_count > 0); | ||
} | ||
|
||
// scope 2 | ||
{ | ||
size_t callback_received_count(0); | ||
|
||
eCAL::string::CSubscriber<std::string> sub_foo("foo"); | ||
auto receive_lambda = [&sub_foo, &callback_received_count](const char* /*topic_*/, const std::string& /*msg*/, long long /*time_*/, long long /*clock_*/, long long /*id_*/) { | ||
std::cout << "Receiving in scope 2" << std::endl; | ||
callback_received_count++; | ||
}; | ||
sub_foo.AddReceiveCallback(receive_lambda); | ||
|
||
// sleep for 2 seconds, we should receive something | ||
std::this_thread::sleep_for(std::chrono::seconds(2)); | ||
|
||
EXPECT_TRUE(callback_received_count > 0); | ||
} | ||
|
||
// stop publishing and join thread | ||
stop_publishing = true; | ||
pub_foo_t.join(); | ||
|
||
// finalize eCAL API | ||
// without destroying any pub / sub | ||
eCAL::Finalize(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to change it, but I would like to make a remark. I strongly dislike the
bool
for ownership vs non-ownership.This should be two different classed, e.g.
OwnedNamedEvent
andNonOwnedNamedEvent
/WeakNamedEvent
.This has two advantages:
a) To everyone reading the code that is using the events, it's clear from the type, if the event is the owner / creator or not
b) You can use inheritance. You don't need to do stuff like
if(m_owner) ...
which makes code clearer and more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, shouldn't the owner always create the event? In the code this is not tied together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to "just fix the issue" and introduce as little change as possible. Let's redesign this in a separate issue.
The same for the creation. The behavior is now exact the same as before. The event is created by the first call of OpenEvent independently from the new ownership attribute. The ownership only plays a role in the destruction phase.