modem: cmux: Define encoding and decoding functions for commands

[SeppoTakalo]

Instead of relying non-standard compiler behavior, define encode and decode functions for all CMUX command structures.

Drop:

struct modem_cmux_command *modem_cmux_command_wrap(const uint8_t *data);
static int modem_cmux_wrap_command(struct modem_cmux_command **command, const uint8_t *data,
				   uint16_t data_len);

And replace with:

static struct modem_cmux_command_type modem_cmux_command_type_decode(const uint8_t byte);
static uint8_t modem_cmux_command_type_encode(const struct modem_cmux_command_type type);
static struct modem_cmux_command_length modem_cmux_command_length_decode(const uint8_t byte);
static uint8_t modem_cmux_command_length_encode(const struct modem_cmux_command_length length);
static struct modem_cmux_command modem_cmux_command_decode(const uint8_t *data, size_t len);
static uint8_t *modem_cmux_command_encode(struct modem_cmux_command *command, uint16_t *len);

as previous _wrap_ functions were relying on:

struct modem_cmux_command_type {
	uint8_t ea: 1;
	uint8_t cr: 1;
	uint8_t value: 6;
};

to be placed correctly. But as discussed on PR #97569 here this is non-standard behavior.
Works on GCC on Arm and seem to be working on GCC on X86_64 native_sim, but we should not rely on that.

Fixes #98420

[SeppoTakalo]

Fixed the compilation issues.
That was a real bug... not GCC failure.

Looks like we need to have static buffer instead of flexible array member, because we end up decoding the whole command and its included data.

[SeppoTakalo]

@bjarki-andreasen amended the PR to contain asserts and validation functions for commands

static bool modem_cmux_command_type_is_valid(const struct modem_cmux_command_type type);
static bool modem_cmux_command_length_is_valid(const struct modem_cmux_command_length length);
static bool modem_cmux_command_is_valid(const struct modem_cmux_command *command);

So type and length field and their EA bits are checked as well as length of all known commands.
This is used for both incoming and outgoing commands.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[SeppoTakalo]

Ping @rerickson1 can you review please?

I would like to rebase my #97362 on top of this, once merged.

[cfriedt]

Just marking DNM because this does not seem to be a bugfix

[SeppoTakalo]

@cfriedt

Just marking DNM because this does not seem to be a bugfix

I would like to disagree with your conclusion.
As I indicated in my PR description, this removes code that relies on bitfields in structures to be packed on a certain order. This behavior, even if work OK on current GCC, is implementation-defined so it is a behavior we should not trust.

As a reference C11 standard: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
section 6.7.2.1 Structure and union specifiers, rule 11:

11 An implementation may allocate any addressable storage unit large enough to hold a bitfield.
If enough space remains, a bit-field that immediately follows another bit-field in a
structure shall be packed into adjacent bits of the same unit. If insufficient space remains,
whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is
implementation-defined. The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined. The alignment of the
addressable storage unit is unspecified.

The important part are the two last sentences where this behavior is implementation-defined and unspecified.

I'm a bit deadblocked here. My previous PR was blocked by a maintainer of that area when I was submitting a code that used bitfields to unpack a binary protocol. How should I deal with my follow up PR:s?

Can I use a code that assumes bitfield to be packed on low-endian order, or not?

[rerickson1]

@cfriedt

Just marking DNM because this does not seem to be a bugfix

I would like to disagree with your conclusion. As I indicated in my PR description, this removes code that relies on bitfields in structures to be packed on a certain order. This behavior, even if work OK on current GCC, is implementation-defined so it is a behavior we should not trust.

As a reference C11 standard: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf section 6.7.2.1 Structure and union specifiers, rule 11:

11 An implementation may allocate any addressable storage unit large enough to hold a bitfield.
If enough space remains, a bit-field that immediately follows another bit-field in a
structure shall be packed into adjacent bits of the same unit. If insufficient space remains,
whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is
implementation-defined. The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined. The alignment of the
addressable storage unit is unspecified.

The important part are the two last sentences where this behavior is implementation-defined and unspecified.

I'm a bit deadblocked here. My previous PR was blocked by a maintainer of that area when I was submitting a code that used bitfields to unpack a binary protocol. How should I deal with my follow up PR:s?

Can I use a code that assumes bitfield to be packed on low-endian order, or not?

This is a very good point, it is fixing issues with code in the tree. Please file an issue and tag this PR in the issue so we can follow the process to get this in during feature freeze.

[cfriedt]

@cfriedt

Just marking DNM because this does not seem to be a bugfix

I would like to disagree with your conclusion.

@SeppoTakalo - thanks for clarifying. It was not obvious to me based on the PR description (when I looked at it last time)

[jhedberg]

I was just about to set the 4.4 milestone, when I noticed the latest comments. Please set the 4.3 milestone to indicate that you consider this for the upcoming release.

[SeppoTakalo]

I was just about to set the 4.4 milestone, when I noticed the latest comments. Please set the 4.3 milestone to indicate that you consider this for the upcoming release.

If there is a feature freeze now, then I think this is fine for waiting, as I don't get the #97362 in either.

This whole PR is about changing something that works as of now, on the compilers that we use today.
So I don't think it as a high priority for given release.

[jhedberg]

Ok, I've added the 4.4 milestone now. That also helps remove some "noise" from https://merge-list.zephyrproject.io since this PR will not show up there until after the final 4.3 release.