Use directory as bagfile and add additonal record options #43
Conversation
ros2bag/ros2bag/verb/record.py
Outdated
| return | ||
| return 'Invalid choice: Can not specify topics and -a at the same time.' | ||
|
|
||
| uri = args.output if args.output else datetime.datetime.now().strftime("%Y_%m_%d-%H_%M_%S") |
There was a problem hiding this comment.
I would recommend prefixing the timestamped folder. Something like rosbag2_[recording_]_YYYY_MM_DD-HH_MM_SS
There was a problem hiding this comment.
Good idea, we will use rosbag2_YYYY_MM_DD-HH_MM_SS.
| @@ -168,6 +175,11 @@ void SqliteStorage::fill_topics_and_types() | |||
| } | |||
| } | |||
|
|
|||
| std::string SqliteStorage::get_database_name(const std::string & uri) | |||
| { | |||
| return rosbag2_storage::FilesystemHelper::get_folder_name(uri) + ".db3"; | |||
There was a problem hiding this comment.
What happens if I put a .db3 file in a new folder and want to open this one? LIke shifting backfiles around?
.../my_new_folder/my_old_bag.db3
There was a problem hiding this comment.
At the moment it is required that the database file is named like the bag folder.
We can change this behavior as soon as the metadata file is available (PR #45).
After that we should use the file path provided by the metadata file to locate the database file.
This path then has to be adjusted after renaming/moving the database file.
There was a problem hiding this comment.
Like shifting bagfiles around?
Generally, we should consider the folder being the bagfile.
So the user should ideally never move/rename/modify the files inside a bag, except when he knows exactly what he does.
There was a problem hiding this comment.
I somewhat agree with it, but I also don't see a strong reason why we should enforce it. I could imagine having an old bagfile around and put it into a new folder. Then I should still be able to load this file.
Also in the long term run, whenever there are split rosbag files, this strong naming match won't hold anymore as every individual bag file will have some sort of pre-/postfix attached to it.
Just for now, I don't see why we should enforce this on code level. Given a URI, I should always be able to extract the folder name independant from the database file.
There was a problem hiding this comment.
I somewhat agree with it, but I also don't see a strong reason why we should enforce it. I could imagine having an old bagfile around and put it into a new folder. Then I should still be able to load this file.
The problem is that we do not have any metadata at this point.
So I see no way how rosbag2 should be able to know that a file in a folder can be understood as a database file.
That's the reason why we enforce this naming scheme for now.
As soon as the info PR lands we have the metadata file which contains the relative file path to the database and can use this path to locate the database.
Also in the long term run, whenever there are split rosbag files, this strong naming match won't hold anymore as every individual bag file will have some sort of pre-/postfix attached to it.
This is also solved using the relative file paths from the metadata file.
This case is the reason why we added the relative_file_paths array to the metadata file.
Just for now, I don't see why we should enforce this on code level. Given a URI, I should always be able to extract the folder name independant from the database file.
Since the database file naming is solely handled by the storage plugin I don't see how this can be achieved through the general rosbag2 interface.
|
|
||
| using namespace ::testing; // NOLINT | ||
|
|
||
| #ifdef _WIN32 |
There was a problem hiding this comment.
with all that #ifdef, does it make sense to include two different helper files (one for win, one for unix) and only separate them once at the beginning?
#ifdef _WIN32
#include process_execution_helpers_windows.hpp
#else
#include process_execution_helpers_unix.hpp
#endif
There was a problem hiding this comment.
The current implementation has one big advantage:
It hides the fact that processes behave differently on windows and unix.
The includer therefore does not have to consider OS details and simply includes the only header available.
There was a problem hiding this comment.
Sorry, what I meant was having three files:
process_execution_helpers.hppprocess_execution_helpers_windows.hppprocess_execution_helpers_unix.hpp
The first one having the code snippet above included.
There was a problem hiding this comment.
Ah, yes, that's a good idea, I added it.
greimela-si
force-pushed
the
feature/directory_as_bagfile
branch
from
4786ac4 to
ea85da2
Compare
- Use SIGINT instead of SIGTERM which is the correct equivalent to pressing Ctrl-C within a shell - Use exec instead of system inside the forked test child process. - Check the return code of the child after the waitpid instead of after the call to system. Using system inside a child process is unnecessary as system forks again. Whether the actual "ros2 bag" process was put in the same process group seemed to be shell dependent. Exec avoids these issues.
|
New CI:
|
Store rosbags using a folder, not a file.
This folder contains all files created by the storage backend.
Add record options
--outputto state the output file and--storageto select the storage backend.Default: Use the current timestamp as bag name and sqlite3 as storage backend.
CI: