Refactor link capabilities

[jean-roland]

Similarly to #279, the goal of this refactor is to improve link capabilities modularity in prevision of adding a new transport.

The bitfield is replaced with a Unicast/Multicast;Stream/Datagram "matrix" enum and a is_reliable flag.
The bitfield macros were removed.

This way the if/else + macro are replaced by a switch case that can be expanded easily.

[cguimaraes]

_is_reliable should be a bit in the _capabilities.

[jean-roland]

_capabilities is no longer a bitfield though, eventually capabilities could be a structure including an enum and the reliable flag

[cguimaraes]

The idea of the bitfield was to reduce the overhead. These values will be booleans, so they can be store in a single bit.
If this was discussed within the team, you can ignore. Otherwise, I suggest to be rethought because this was done intentionally.

[cguimaraes]

tagging @p-avital

[jean-roland]

Also invoking @Mallets

[Mallets]

In this specific case I don't think it will make any difference.
capabilities is uint8_t and bool takes the same space of a uint8_t.
Unless you are running on a 8-bit platform, and because of alignment, the two solutions are equivalent w.r.t. space usage.

The bitfield usually is used to express the combination of different independent capabilities, which may be invalid. Looking at the code re-organisation I believe that having specific enum variants instead of bitfields will make the logic more explicit.

[cguimaraes]

As this is zenoh-pico, I see 8-bit as a target.
Also, you are just considering one capability, so you are not being fair in the analysis.

Today, we are considering 3 capabilities: _Z_LINK_IS_STREAMED, _Z_LINK_IS_RELIABLE, _Z_LINK_IS_MULTICAST.
Tomorrow we may have others capabilities like _Z_LINK_IS_SECURE or _Z_LINK_IS_UNIDIRECTIONAL.

I am still in favor of the bitfield, also because it scales better than to enumerate all the possible combinations.

[jean-roland]

Just to weigh in on the discussion, the current bitfield structure isn't super well suited because only RELIABLE is a true flag/bool, for multicast and streamed it really represents enums:

enum LINK_CAPABILITY_TRANSPORT {
    UNICAST,
    MULTICAST,
    <New values>,
}

and

enum LINK_CAPABILITY_FLOW (just made this up) {
    DATAGRAM,
    STREAM,
}

Ideally we would want to have both enums + any flags + new future values as their own separate fields. Of course that would cost more memory

So if we really want to represent things correctly and use as little memory as possible then we have the possibility to do a register like structure:

struct capability {
    uint8_t transport: 2;
    uint8_t flow: 1;
    bool reliable: 1;
    uin8_t reserved: 4;
}

It does open a lot of problems and future headache if we have to change the fields but that's a possibility.

Another one is a compromise with a bitfield and a combined enum which is almost this PR (albeit the bitfield is only the reliable flag). That won't scale well if we the enums grow too much or if we have to add more enums.

[p-avital]

I do like the register-like structure proposal a bit better than the enum one. Not extremely familiar with it, but then couldn't we do

struct capability {
    enum LINK_CAPABILITY_TRANSPORT transport: 2;
    enum LINK_CAPABILITY_FLOW flow: 1;
    _Bool reliable: 1;
    uin8_t reserved: 4;
}

to make each field unambiguous as to its purpose? (since C enums are dumb, this might backfire and make capability int-sized though)

[Mallets]

My main goal is to avoid ambiguity as well as avoiding the possibility to represent invalid states. For that reason, I believe the current PR is a good tradeoff, as mentioned by @jean-roland .

I don't expect the enum to significantly grow and have a large number of variants. We are targeting fundamental characteristics of transports and links, which have an impact on the Zenoh protocol itself...

[cguimaraes]

This should be a bitwise operation as defined in the existing _Z_LINK_IS_STREAMED

[jean-roland]

The goal is to remove the bitwise operations and use a switch as future transport will not necessarily map neatly to the stream/cast separation and it's easier to expand.

[cguimaraes]

Future transports will support or not a given set of functionalities. And the way, the core code should be handled is accordingly to the support or not these functionalities.
As far as I remember, this is how the Rust implementation is also tackling this issue.

Can you give an example of a future transport that does not map? Note that if new properties must be added, they can be done is a clean way with the current approach.

[cguimaraes]

tagging @Mallets

[Mallets]

In accordance with my previous comment, bitwise operations have the tendency to be interpreted as a combination of indipendent parameters. This has led to some confusion and misconception on how things should be done and structured. I believe in this case having a clear variant separation helps in understanding what are the real cardinality of available options.

[jean-roland]

I added MULTICAST_STREAM only because bluetooth was declared multicast and streamed. Tagged you both at the location

[Mallets]

Then I withdraw my comment: As far as I know there are no multicast protocols that are byte-streamed.
Now I know :)

[p-avital]

My take on this is that regardless of the existence of a multicast-streamed transport, I think we always treat multicast-ness and streamed-ness independently (since one affects routing and the other serialization), so while the enum might highlight that some don't exist, is that really a relevant fact we need to encode in here?

In general, I take an enum going through the combinations of X and Y as X_Y to be a sign that these properties are rather independent, angling toward the bitfield. But bitfields are a bit ugly: I like the register-struct proposal @jean-roland made in the other thread better if we end up returning to the bitfield approach.

[Mallets]

Then I propose to move forward with @jean-roland proposal then.

[cguimaraes]

Disclaimer: Bluetooth is not MULTICAST STREAM per se. This was a workaround to make it work with Zenoh-Pico only and for the selected profile, since Zenoh Rust did not support Multicast or Bluetooth.

As @p-avital mentioned, each property addresses two distinct mechanisms within Zenoh, and it was the fact that they were independent that allowed us to be flexible on supporting new protocols even if part of the entire system was not fully supporting them.

I am fine with the suggestion of @jean-roland here ( #280 (comment) ). But note that there are only two transports in Zenoh and, as far as I am aware of the roadmap, there is not any plan to change it further.

[jean-roland]

Changes done, tried to declare the fields as enum types and _Bool but got a 4 bytes structure, so had to declare them all as uint8_t