-
Notifications
You must be signed in to change notification settings - Fork 16
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
[21413] Record data in SQL #173
base: main
Are you sure you want to change the base?
Changes from 33 commits
4811991
192dd2e
12c478f
f5231b7
e534a31
0a8d7d4
c2f6b82
007925a
a7981f0
a68c9ff
a62d977
2f6f271
acb296e
62fb528
8246bc6
89fff92
a2ab83f
b1f2d2f
a97bf55
b39ab01
623b604
37b770b
94372b2
7898099
60ca313
e01c8d7
cb0f53d
6326c16
006d3ce
6ba899f
3014769
bd5271a
d74fe42
c36fd7f
a462998
a3ce840
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 |
---|---|---|
|
@@ -16,8 +16,10 @@ | |
# CMake build rules for DDS Recorder Submodule | ||
############################################################################### | ||
cmake_minimum_required(VERSION 3.5) | ||
set(CMAKE_SYSTEM_VERSION 10.0) | ||
|
||
# Done this to set machine architecture and be able to call cmake_utils | ||
enable_language(C) | ||
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. Maybe this, and the new statement above should only be added to blackbox tests using sqlite. |
||
enable_language(CXX) | ||
|
||
############################################################################### | ||
|
@@ -60,18 +62,8 @@ project( | |
# - Configure log depending on LOG_INFO flag and CMake type | ||
configure_project_cpp() | ||
|
||
file( | ||
GLOB_RECURSE SOURCES_FILES | ||
"${PROJECT_SOURCE_DIR}/src/cpp/tool/*.cpp" | ||
"${PROJECT_SOURCE_DIR}/src/cpp/user_interface/*.cpp" | ||
"${PROJECT_SOURCE_DIR}/src/cpp/main.cpp" | ||
"${PROJECT_SOURCE_DIR}/src/cpp/command_receiver/Command*.cpp" | ||
"${PROJECT_SOURCE_DIR}/src/cpp/*/*/*/*.cxx" | ||
) | ||
|
||
compile_tool( | ||
"${PROJECT_SOURCE_DIR}/src/cpp/" # Source directory | ||
"${SOURCES_FILES}" | ||
) | ||
|
||
compile_test_tool("${PROJECT_SOURCE_DIR}/test/") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,6 +261,8 @@ int main( | |
|
||
// Load configuration from YAML | ||
eprosima::ddsrecorder::yaml::RecorderConfiguration configuration(commandline_args.file_path, &commandline_args); | ||
// Flag to avoid reloading configuration in the first iteration of enable_remote_controller loop | ||
bool config_loaded = true; | ||
|
||
///// | ||
// Logging | ||
|
@@ -297,7 +299,8 @@ int main( | |
logUser(DDSRECORDER_EXECUTION, "DDS Recorder running."); | ||
|
||
// The file tracker must be stored outside of the loop since it is shared between instances | ||
std::shared_ptr<eprosima::ddsrecorder::participants::FileTracker> file_tracker; | ||
std::shared_ptr<eprosima::ddsrecorder::participants::FileTracker> mcap_file_tracker; | ||
std::shared_ptr<eprosima::ddsrecorder::participants::FileTracker> sql_file_tracker; | ||
|
||
if (configuration.enable_remote_controller) | ||
{ | ||
|
@@ -344,7 +347,8 @@ int main( | |
{ | ||
// Save the set of output files from being overwritten. | ||
// WARNING: If set, the resource-limits won't be consistent after stopping the DDS Recorder. | ||
file_tracker.reset(); | ||
mcap_file_tracker.reset(); | ||
sql_file_tracker.reset(); | ||
} | ||
|
||
prev_command = CommandCode::stop; | ||
|
@@ -402,11 +406,18 @@ int main( | |
|
||
// Reload YAML configuration file, in case it changed during STOPPED state | ||
// NOTE: Changes to all (but controller specific) recorder configuration options are taken into account | ||
configuration = eprosima::ddsrecorder::yaml::RecorderConfiguration(commandline_args.file_path); | ||
if(!config_loaded) | ||
{ | ||
configuration = eprosima::ddsrecorder::yaml::RecorderConfiguration(commandline_args.file_path); | ||
} | ||
else | ||
{ | ||
config_loaded = false; | ||
} | ||
Comment on lines
+409
to
+416
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. I am not sure this is an improvement. I agree it would be better to avoid two immediate consecutive configuration loads, as it is the case when the initial state is running or paused. But a long time might have passed when coming from a stopped state, and the user might have changed the allowlist in the meantime. |
||
|
||
// Create DDS Recorder | ||
auto recorder = std::make_unique<DdsRecorder>( | ||
configuration, initial_state, close_handler, file_tracker); | ||
configuration, initial_state, close_handler, mcap_file_tracker, sql_file_tracker); | ||
|
||
// Create File Watcher Handler | ||
std::unique_ptr<eprosima::utils::event::FileWatcherHandler> file_watcher_handler; | ||
|
@@ -540,7 +551,7 @@ int main( | |
{ | ||
// Start recording right away | ||
auto recorder = std::make_unique<DdsRecorder>( | ||
configuration, DdsRecorderState::RUNNING, close_handler, file_tracker); | ||
configuration, DdsRecorderState::RUNNING, close_handler, mcap_file_tracker, sql_file_tracker); | ||
|
||
// Create File Watcher Handler | ||
std::unique_ptr<eprosima::utils::event::FileWatcherHandler> file_watcher_handler; | ||
|
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 am not sure this is the best approach. First, apparently this only makes sense in Windows, so it should be within an if. I also wonder if we should be imposing the sdk version here or we should investigate which versions are compatible with sqlite and warn the user she should be compiling with one of those (and specify it via cmake args). Otherwise, we could be forcing a version not available in a user's machine when another compatible one is present.