Skip to content
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

WIP: Protocol 0.8 #223

Merged
merged 121 commits into from
Aug 31, 2023
Merged

Conversation

p-avital
Copy link
Contributor

No description provided.

@p-avital p-avital marked this pull request as ready for review August 25, 2023 16:59
int8_t _z_bytes_init(_z_bytes_t *bs, size_t capacity);
_z_bytes_t _z_bytes_make(size_t capacity);
_z_bytes_t _z_bytes_wrap(const uint8_t *bs, size_t len);
_z_bytes_t _z_bytes_steal(_z_bytes_t *b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't '_z_bytes_move` already implementing the same behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was indeed, didn't spot it because the behaviour differs to other moves. I think _z_bytes_move will be renamed to the steal version, to avoid being confused for the move convention

@@ -23,7 +25,7 @@
typedef struct {
_z_value_t _value;
_z_keyexpr_t _key;
_z_zint_t _qid;
uint32_t _rid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

z_declare_resource returns an uint16_t-size id.
Any reason for a wider range here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, rid stands for request id. Changing the name to avoid confusions

@@ -33,7 +35,7 @@ typedef struct {
* Return type when declaring a queryable.
*/
typedef struct {
_z_zint_t _id;
uint32_t _id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment here, but I did not yet check the qid return size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here is an entity id, they're u32

#include "zenoh-pico/net/session.h"
#include "zenoh-pico/protocol/core.h"

/**
* Return type when declaring a subscriber.
*/
typedef struct {
_z_zint_t _id;
uint32_t _id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here as for the queryable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entity_id, u32

src/link/link.c Outdated
@@ -148,8 +148,7 @@ size_t _z_link_recv_exact_zbuf(const _z_link_t *link, _z_zbuf_t *zbf, size_t len

int8_t _z_link_send_wbuf(const _z_link_t *link, const _z_wbuf_t *wbf) {
int8_t ret = _Z_RES_OK;

for (size_t i = 0; (i < _z_wbuf_len_iosli(wbf)) || (ret == -1); i++) {
for (size_t i = 0; (i < _z_wbuf_len_iosli(wbf)); i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if an iosli fails to send and a subsequent one succeeds, this function will return _Z_RES_OK and it shall not be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so here, the behaviour should depend on whether or not the link is streamed:

  • on streamed links, keep trying to send what hasn't been sent.
  • on datagram links, give up after the first failure, as only fragmented payloads have multi-slice wbufs, which means missing one is a full on transmit failure for the whole thing

return ret;
}
_z_id_t _z_id_empty() {
return (_z_id_t){.id = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm, but I think: return (_z_id_t){.id = {0}}; would initialize the entire array with 0s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that works for structures, I have a small doubt on arrays... needs double-checking just in case

Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a quick and light review on my side.
I suggest @Mallets to do a more detailed one, including double checking the message formats and constant values with the Rust implementation.

@p-avital p-avital changed the base branch from master to new-protocol August 31, 2023 14:33
@p-avital p-avital merged commit 88cb8ec into eclipse-zenoh:new-protocol Aug 31, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants