rcl_yaml_param_parser: add support for binary tag to load byte array parameter#1256
Conversation
|
@romainreignier Thank you for this PR 👍 |
|
In YAML files, binary data is encoded using base64. As for decoding base64, we'd like to hear your thoughts. Is it better to write our own decoder or use a library like libb64 or OpenSSL? |
|
@fujitatomoya Friendly ping |
fujitatomoya
left a comment
There was a problem hiding this comment.
i'd recommend a self-contained C implementation in rcutils with following reasons.
- no new external dependency
- base64 decode is trivial, easily implemented in rcutrils which already has similar code.
- OpenSSL is too heavy for this use case...
|
@romainreignier are you still working on this? |
Not currently, I have missed your previous replies. |
Maybe I make you misunderstood. I just took over this #1032. However, #1032 does not include support for the binary tag. Once this PR is merged, full support for all tags will be available. I think the next step is to add base64 decode/encode APIs to rcutils first. |
|
Yes please, if you can implement it in rcutils, I will update this PR. |
|
Base64 decode/encode APIs had been added ros2/rcutils#533. About how to use, you can refer to test code Please update this PR. |
9f34bee to
d5629ac
Compare
|
@Barry-Xu-2018 I have updated the PR using In |
| for (size_t i = 0; i < param_var->byte_array_value->size; i++) { | ||
| if (param_var->byte_array_value->values) { | ||
| printf( | ||
| "0x%02x, ", | ||
| param_var->byte_array_value->values[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
There’s no need to check param_var->byte_array_value->values in every iteration of the loop.
if (param_var->byte_array_value->values) {
for (size_t i = 0; i < param_var->byte_array_value->size; i++) {
printf(
"0x%02x, ",
param_var->byte_array_value->values[i]);
}
}There was a problem hiding this comment.
Yes, I agree, but it is done like this for arrays of bool, integer, double and char (string).
Should I change it for the other types?
In case values is NULL, we just don't print anything?
There was a problem hiding this comment.
Should I change it for the other types?
I think these issues should be addressed in a separate PR rather than being included in your PR.
In case values is NULL, we just don't print anything?
Yes. I think it is already incorrect when values is NULL, and an error should have already been reported elsewhere in the code.
d5629ac to
d9d27e4
Compare
|
Thanks for the review @Barry-Xu-2018 |
|
Note, the CI fails because it does not use a version of rcutils with base64. |
Barry-Xu-2018
left a comment
There was a problem hiding this comment.
Some more comments.
… parameters This allows to pass byte array parameters using the YAML [binary tag](https://yaml.org/spec/1.2.2/#24-tags) Example: ``` ros2 run pkg_name exe_name --ros-args -p 'calib:=!!binary AAECAw==' ``` Address ros2/ros2#1436 Can be seen as part of ros2#1026 Signed-off-by: Romain Reignier <romain.reignier@exail.com>
d9d27e4 to
80368f8
Compare
|
Thanks a lot @Barry-Xu-2018 for your patience and careful review. |
|
It’s totally fine 😄 |
|
Pulls: #1256 |
|
All the failures were caused by errors while compiling the urdfdom_headers package. |
|
Pulls: #1256 |
@Barry-Xu-2018 CI should have been run with |
|
Pulls: #1256 |
Yes. Build configuration was wrong. Thank you for your reminder. |
Description
When a parameter is declared as an array of bytes (
std::vector<uint8_t>), passing an array of integers in the command line or in a YAML file is interpreted by the YAML parser as an array of integers and produces an error.To workaround this, I have used a base64 encoded string with the the
!!binarytag as shown in the YAML specification.Example:
Note that this implementation adds a dependency on OpenSSL which might not be acceptable (and the ugly global variable hack to use the custom allocator).
We might implement the base64 decoding directly in C or use another lib like libb64.
Until this question is resolved, I put this PR as a Draft.
The sequence handling is not very clear in
parse.cand might need some improvements.Fixes ros2/ros2#1436
Can be seen as part of #1026
Is this user-facing behavior change?
Yes, the byte array parameters are now supported as ros arguments or in yaml parameters files.
Until now, it was not supported at all, has decribed in ros2/ros2#1436.
Did you use Generative AI?
Yes, the
base64_decode()function has been written with the help of Mistral Le Chat.