Add service client support.

[g4sp3r]

This PR adds service client structures and ability to call ROS services. Inspired by #7

Example usage in examples/srv_client_add2ints.c
Tested only on Linux for now. Used ros2 run demo_nodes_cpp add_two_ints_server on ROS host.

Fix generated hashes for service types. Generator script was using hashes from _Request and _Response types.

[g4sp3r]

I get segmentation fault when setting Z_CONSOLIDATION_MODE_MONOTONIC option and first query reply is received.
Looks to be coming from null pointer de-reference in _z_keyexpr_move called by _z_trigger_query_reply_partial_inner.

[g4sp3r]

Still not totally sure about service client _in_progress handling and limiting the picoros_service_call to one ongoing call per client.

On one hand without it there is a situation where application can call picoros_service_call multiple times and receive multiple reply callbacks at later time. I don't think this is what you want in majority of applications but I could be mistaken.

Alternatively if this was removed, client passed to picoros_service_call can go out of scope right after the return.
Not very critical in my opinion, for embedded scenarios statically allocated client structure is usually preferred.

---

Service reply drop callback could be added to client structure so application would be informed of this case.

[xopxe]

After doing git checkout origin/service_call and compiling I'm getting this. Any idea?
(Arduino under Platformio)

...
Compiling .pio/build/pico32/lib955/Micro-CDR/c/types/sequence.c.o
Compiling .pio/build/pico32/lib955/Micro-CDR/c/types/string.c.o
Compiling .pio/build/pico32/lib210/Pico-ROS-software/picoparams.c.o
Compiling .pio/build/pico32/lib210/Pico-ROS-software/picoros.c.o
Compiling .pio/build/pico32/lib210/Pico-ROS-software/picoserdes.c.o
.pio/libdeps/pico32/Pico-ROS-software/src/picoros.c: In function 'picoros_service_call':
.pio/libdeps/pico32/Pico-ROS-software/src/picoros.c:476:5: warning: implicit declaration of function 'z_closure'; did you mean 'z_close'? [-Wimplicit-function-declaration]
     z_closure(&callback, get_data_handler, get_drop_handler, client);
     ^~~~~~~~~
     z_close
.pio/libdeps/pico32/Pico-ROS-software/src/picoros.c:482:9: warning: implicit declaration of function 'z_drop'; did you mean 'z_free'? [-Wimplicit-function-declaration]
         z_drop(opts->attachment);
         ^~~~~~
         z_free

[g4sp3r]

Hey @xopxe, thanks for testing. Looks like zenoh-pico/api/macros.h is not available in c++. I tried to fix it in the last commit.

[g4sp3r]

Actually I am wrong. There are c++ overloaded functions declared a bit further down. So it seems it is something with the includes?

[xopxe]

Just before reading the last comment I pulled the "Fix for c++" and it compiled succesfully.
If it had to compile without this, it could be the includes, as you say, or perhaps the build was not cleaning correctly. I'll look into it.

[xopxe]

I've been testing, and could it be that the namespace for the key expression is actually in node->name?

diff --git a/src/picoros.c b/src/picoros.c
index 7fc9186..7ed5b35 100644
--- a/src/picoros.c
+++ b/src/picoros.c
@@ -70,8 +70,8 @@ static int rmw_zenoh_topic_keyexpr(picoros_node_t* node, rmw_topic_t* topic, cha
 }
 
 static int rmw_zenoh_service_keyexpr(picoros_node_t* node, rmw_topic_t* topic, char* keyexpr) {
-    if (topic->name == NULL){
-        return snprintf(keyexpr, KEYEXPR_SIZE, "%" PRIu32 "/%s/%s_/RIHS01_%s", node->domain_id, node->name,
+    if (node->name == NULL){
+        return snprintf(keyexpr, KEYEXPR_SIZE, "%" PRIu32 "/%s/%s_/RIHS01_%s", node->domain_id, topic->name,
          topic->type, topic->rihs_hash);
     }

Also, it seems that the key is recomputed on every service call. I see that "native" RCL clients have a create_client call, which I suppose computes them once. Perhaps it would be worthwhile to mimic that design?

[g4sp3r]

Hey @xopxe. Your questions actually lead me to see that we were missing topic and service name separator conversion when making key expressions. It is added in the latest commit. I don't know if this was your issue, please let me know.
The idea is that node->name (node_name in service call) takes only the name of node. The topic->name takes the full topic path. I was checking if topic->name is not set to skip it (the case of add2Ints service in ROS).

Very good point about generating the key only once. What do you think about generating it the first time and saving in the service structure. Then check on each service call if we have it already. Like that user doesn't need to call any new function.

EDIT:
Thinking more your proposal is better for embedded scenarios

[g4sp3r]

@xopxe I checked again the services in ROS when running ros2 run demo_nodes_cpp add_two_ints_server

/add_two_ints
/add_two_ints_server/describe_parameters
/add_two_ints_server/get_parameter_types
/add_two_ints_server/get_parameters
/add_two_ints_server/get_type_description
/add_two_ints_server/list_parameters
/add_two_ints_server/set_parameters
/add_two_ints_server/set_parameters_atomically

in this case add_two_ints_server is the node.

So in current version it's not supported to create service server key expression without node (you could now set node name pointer to 0 before declaring the service). The other ones are supported.
Calling is possible with all the above services.
For /add_two_ints service the node_name is then not set in service call structure.

---

You spotted correctly we should actually check for existence of node->name. Thank you!

[g4sp3r]

Sorry for the noise, after checking better I see the topic and service key expressions don't need to be mangled so I removed that part.