Add query & queryable token config #260
Conversation
There was a problem hiding this comment.
If the purpose is to make the final binary smaller, see inline comments.
Also, it was always a mindset in Zenoh team that errors or incompatibilities to be explicit to the user (preferable at compilation time, instead of runtime). Then, I suggest that if queries /queryables are disable that the corresponding API is also ommited.
src/api/api.c
Outdated
| @@ -654,14 +657,17 @@ typedef struct __z_reply_handler_wrapper_t { | |||
| } __z_reply_handler_wrapper_t; | |||
|
|
|||
| void __z_reply_handler(_z_reply_t *reply, __z_reply_handler_wrapper_t *wrapped_ctx) { | |||
There was a problem hiding this comment.
If queries are not supported, the entire reply handler can be ommitted.
src/api/api.c
Outdated
| z_owned_reply_t oreply = {._value = reply}; | ||
|
|
||
| wrapped_ctx->user_call(&oreply, wrapped_ctx->ctx); | ||
| z_reply_drop(&oreply); // user_call is allowed to take ownership of the reply by setting oreply._value to NULL | ||
| #endif | ||
| } | ||
|
|
||
| int8_t z_get(z_session_t zs, z_keyexpr_t keyexpr, const char *parameters, z_owned_closure_reply_t *callback, |
There was a problem hiding this comment.
If queries are disabled, I would completely omit any related APIs.
In doing so, the user will have an explicit error at compilation. Otherwise, it will become harder for users (and us) to debug non-compliant behavior.
Also, you can save some extra bytes in the final binary.
src/api/api.c
Outdated
| @@ -896,6 +905,7 @@ z_queryable_options_t z_queryable_options_default(void) { | |||
|
|
|||
| z_owned_queryable_t z_declare_queryable(z_session_t zs, z_keyexpr_t keyexpr, z_owned_closure_query_t *callback, | |||
There was a problem hiding this comment.
If queryables are disabled, I would completely omit any related APIs.
In doing so, the user will have an explicit error at compilation. Otherwise, it will become harder for users (and us) to debug non-compliant behavior.
Also, you can save some extra bytes in the final binary.
src/api/api.c
Outdated
| @@ -923,14 +933,20 @@ z_owned_queryable_t z_declare_queryable(z_session_t zs, z_keyexpr_t keyexpr, z_o | |||
|
|
|||
| return (z_owned_queryable_t){ | |||
| ._value = _z_declare_queryable(zs._val, key, opt.complete, callback->call, callback->drop, ctx)}; | |||
| #else | |||
| return (z_owned_queryable_t){._value = NULL}; | |||
| #endif | |||
| } | |||
|
|
|||
| int8_t z_undeclare_queryable(z_owned_queryable_t *queryable) { | |||
There was a problem hiding this comment.
If queryables are disabled, I would completely omit any related APIs.
In doing so, the user will have an explicit error at compilation. Otherwise, it will become harder for users (and us) to debug non-compliant behavior.
Also, you can save some extra bytes in the final binary.
src/api/api.c
Outdated
| @@ -940,6 +956,7 @@ z_query_reply_options_t z_query_reply_options_default(void) { | |||
|
|
|||
| int8_t z_query_reply(const z_query_t *query, const z_keyexpr_t keyexpr, const uint8_t *payload, size_t payload_len, | |||
| const z_query_reply_options_t *options) { | |||
There was a problem hiding this comment.
If queryables are disabled, I would completely omit any related APIs.
In doing so, the user will have an explicit error at compilation. Otherwise, it will become harder for users (and us) to debug non-compliant behavior.
Also, you can save some extra bytes in the final binary.
src/api/api.c
Outdated
| @@ -949,6 +966,9 @@ int8_t z_query_reply(const z_query_t *query, const z_keyexpr_t keyexpr, const ui | |||
| }, | |||
| .encoding = {.prefix = opts.encoding.prefix, .suffix = opts.encoding.suffix}}; | |||
| return _z_send_reply(query, keyexpr, value); | |||
| #else | |||
| return _Z_ERR_GENERIC; | |||
| #endif | |||
| } | |||
|
|
|||
| _Bool z_reply_is_ok(const z_owned_reply_t *reply) { | |||
There was a problem hiding this comment.
If queryables are disabled, I would completely omit any related APIs.
In doing so, the user will have an explicit error at compilation. Otherwise, it will become harder for users (and us) to debug non-compliant behavior.
Also, you can save some extra bytes in the final binary.
src/session/rx.c
Outdated
| @@ -154,9 +162,13 @@ int8_t _z_handle_network_message(_z_session_t *zn, _z_zenoh_message_t *msg, uint | |||
| } | |||
| } break; | |||
| case _Z_N_RESPONSE_FINAL: { | |||
| #if Z_FEATURE_QUERIES == 1 | |||
There was a problem hiding this comment.
Response and ResponseFinal are not limited to queries/queryables.
|
Here are the modifications on the update:
The changes slightly enhanced the size reduction:
|
| #ifndef ZENOH_PICO_SESSION_REPLY_H | ||
| #define ZENOH_PICO_SESSION_REPLY_H | ||
|
|
||
| int8_t _z_trigger_reply_partial(_z_session_t *zn, _z_zint_t id, _z_keyexpr_t key, _z_msg_reply_t *reply); |
There was a problem hiding this comment.
Check for the features here as well.
There was a problem hiding this comment.
The overarching goal here, as mentioned by @Mallets, is to dissociate replies with requests as in the future there might be reply packets for other features.
Same for push packet and subscriptions.
| #if Z_FEATURE_QUERY == 1 | ||
| ret = _z_trigger_query_reply_partial(zn, id, key, reply->_value.payload, reply->_value.encoding, Z_SAMPLE_KIND_PUT, | ||
| reply->_timestamp); | ||
| #endif |
There was a problem hiding this comment.
Return error if not supported?
|
|
||
| #if Z_FEATURE_QUERY == 1 | ||
| _z_trigger_query_reply_final(zn, id); | ||
| #endif |
There was a problem hiding this comment.
Return error if not supported?
This adds compile time config tokens to add or remove queries & queryables independently. The measured impact on program memory was, if you look at the sum of the symbol sizes from the Z_pub example elf, in O3 on amd64 linux: