Bluetooth: Mesh: Add separate macro define for opcode

[LingaoM]

Use separate macro define for mesh message buf define. And remove duplicate init.

[PavelVPV]

I'd recommend to remove these macros.

[jhedberg]

@LingaoM the latest iteration of the PR is closer to what I'd consider acceptable, thanks! However, it's really a pity that we'd have to effectively double the opcode definition lists with two declarations for each opcode, when in principle all the information we need is already in the original declarations.

Did you explore the possibility of having the BT_MESH_MODEL_OP_*() macros expand to byte arrays instead of integers? You'd still likely want a macro to convert that into the uint32_t opcode member of struct bt_mesh_model_op, but if the opcode macros expanded to uint8_t arrays then you should be able to apply sizeof(OP_FOO) on them to get their size at build-time. I haven't tested it (which means there might be something I'm overlooking), but the macros would then look something like the following:

#define BT_MESH_MODEL_OP_1(b0) ((uint8_t[]){ (b0) })
#define BT_MESH_MODEL_OP_2(b0, b1) ((uint8_t[]){ (b0), (b1) })
#define BT_MESH_MODEL_OP_3(b0, cid) ((uint8_t[]){ (b0), ((cid) >> 8), ((cid) & 0xff) })

[LingaoM]

Did you explore the possibility of having the BT_MESH_MODEL_OP_*() macros expand to byte arrays instead of integers?

I think this modification is more complicated, have you considered that the opcode is also referenced elsewhere in the code.

[LingaoM]

but if the opcode macros expanded to uint8_t arrays then you should be able to apply sizeof(OP_FOO) on them to get their size at build-time.

What we need is in the preprocessing-time rather than build-time.

[jhedberg]

but if the opcode macros expanded to uint8_t arrays then you should be able to apply sizeof(OP_FOO) on them to get their size at build-time.

What we need is in the preprocessing-time rather than build-time.

Yes, you're right. I overlooked that part. There are a couple of subsystems in Zephyr where advanced preprocessor macro tricks (aka macrobatics) are heavily used, e.g. the logging subsystem as well as device tree. It would be nice if we could get some of the developers in those areas to take a look at this case, since I still haven't given up the hope that there exists a way to keep both the definition and usage of the opcodes as simple as they are now :)

[jhedberg]

What we need is in the preprocessing-time rather than build-time.

Actually, are you sure about that? I thought the main idea of this PR was to convert runtime message init to build time init? It's true that you can't use sizeof() as part of a preprocessor conditional directive, however you can use it as part of C code if-branches, and since the value is build-time evaluated, the linker will optimize out any branches from the final code which will never be taken (same idea as e.g. with IS_ENABLED()).

[LingaoM]

Actually, are you sure about that? I thought the main idea of this PR was to convert runtime message init to build time init? It's true that you can't use sizeof() as part of a preprocessor conditional directive, however you can use it as part of C code if-branches, and since the value is build-time evaluated, the linker will optimize out any branches from the final code which will never be taken (same idea as e.g. with IS_ENABLED()).

Hi @jhedberg if use it as part of C code if-branches, something like this:

	if (BT_MESH_MODEL_OP_LEN(_op) == 2) {
		NET_BUF_SIMPLE_DEFINE(msg, ....);
         } else {
		NET_BUF_SIMPLE_DEFINE(msg, ....);
	}

	key_idx_pack_pair(&msg, net_idx, app_idx);

Obvious, The compiler will report build error:

error: 'msg' undeclared (first use in this function)
  299 |         key_idx_pack_pair(&msg, net_idx, app_idx);
      |                            ^~~

[jhedberg]

Hi @jhedberg if use it as part of C code if-branches, something like this:

	if (BT_MESH_MODEL_OP_LEN(_op) == 2) {
		NET_BUF_SIMPLE_DEFINE(msg, ....);
         } else {
		NET_BUF_SIMPLE_DEFINE(msg, ....);
	}

	key_idx_pack_pair(&msg, net_idx, app_idx);

Obvious, The compiler will report build error:

error: 'msg' undeclared (first use in this function)
  299 |         key_idx_pack_pair(&msg, net_idx, app_idx);
      |                            ^~~

One thought that came to mind is to have just a single NET_BUF_SIMPLE_DEFINE() statement and then use the C ternary operator inside it (len == 1 ? ... : ...), however I think the problem with that is that then this might not be usable outside of a function (it's currently allowed to declare net_buf_simple objects as global/static variables).

[LingaoM]

C ternary operator

_op == 3: b1, b2, b3 : ...

This method still does not work, because the opcode is split into a byte list, and it is an unexpected symbol(,)in the ternary operation.

[jhedberg]

C ternary operator

_op == 3: b1, b2, b3 : ...

This method still does not work, because the opcode is split into a byte list, and it is an unexpected symbol(,)in the ternary operation.

That's because of the way that you've extended the NET_BUF_SIMPLE_DEFINE() macro with __VA_ARGS__. You'd need to modify it so that it only takes one optional argument which needs to evaluate to a valid uint8_t array in itself. I.e. instead of this:

(uint8_t net_buf_data_##_name[_size] = { __VA_ARGS__ });

you'd have something like this:

(uint8_t net_buf_data_##_name[_size] = (uint8_t[])(__VA_ARGS__));

E.g. the following is valid C code (I don't get any warnings):

	const uint32_t op = 0x0000ffff;
	struct {
		uint8_t *buf;
	} b = {
		.buf = op > 0xff ? ((uint8_t[]){ (op >> 8), (op & 0xff) }) : ((uint8_t[]){ (op & 0xff) }),
	};

[jhedberg]

Not sure if it came up as part of earlier reviews, but couldn't you remove the COND_CODE_1() and instead always just do:

uint8_t net_buf_data_##_name[_size] = { __VA_ARGS__ };

Even if __VA_ARGS__ is empty that should still be valid code. The difference in behaviour to the previous version is that now we'd always explicitly initialize the content of the data array, whereas previously this would have been uninitialised memory.

[LingaoM]

I don't want to do this because it will significantly increase the flash size. And will cause the default behavior to change. initialize the content of the 0, whereas previously this would have been uninitialised memory.

[jhedberg]

That's because of the way that you've extended the NET_BUF_SIMPLE_DEFINE() macro with __VA_ARGS__. You'd need to modify it so that it only takes one optional argument which needs to evaluate to a valid uint8_t array in itself. I.e. instead of this:

(uint8_t net_buf_data_##_name[_size] = { __VA_ARGS__ });

you'd have something like this:

(uint8_t net_buf_data_##_name[_size] = (uint8_t[])(__VA_ARGS__));

@LingaoM I just realised this won't work, because of the way you're using two levels of __VA_ARGS__ where the first comes from the BT_MESH_MODEL_BUF_INIT() macro and the second from the NET_BUF_SIMPLE_DEFINE() macro, and both are used as input into a single uint8_t array.

[jhedberg]

@LingaoM I suspect your current proposal with *_LEN defines is as good as it gets, as long as we want to keep the OP_* defines as integers. I was experimenting a bit with the Zephyr util_macro.h helpers to see how things would look like if we used arrays instead. I'm not saying should go this route, but it's good to at least understand what it'd in practice mean. With the following, the idea is that the opcodes are defined using either BT_MESH_MODEL_OP_STD() (for standard opcodes) or BT_MESH_MODEL_OP_VND() (for vendor opcodes). These would expand to arrays and could be directly used with the message macros that you're working on. When there's a need to translate to the integer (uint32_t) format, the values would need to be passed through the BT_MESH_MODEL_OPCODE() macro.

#define Z_OP_1(...) (GET_ARG_N(1, __VA_ARGS__))
#define Z_OP_2(...) ((Z_OP_1(__VA_ARGS__) << 8) | (GET_ARG_N(2, __VA_ARGS__)))
#define Z_OP_3(...) (0xc00000 | (Z_OP_2(__VA_ARGS__) << 8) | (GET_ARG_N(3, __VA_ARGS__)))

#define BT_MESH_MODEL_OPCODE(...) COND_CODE_0(NUM_VA_ARGS_LESS_1(__VA_ARGS__),                    \
					      Z_OP_1(__VA_ARGS__),                                \
					      COND_CODE_1(NUM_VA_ARGS_LESS_1(__VA_ARGS__),        \
							  Z_OP_2(__VA_ARGS__, dummy),             \
							  Z_OP_3(__VA_ARGS__, dummy, dummy)))

#define BT_MESH_MODEL_OP_STD(b0, ...) COND_CODE_0(NUM_VA_ARGS_LESS_1(_, ##__VA_ARGS__),           \
						  (b0), ((b0), (GET_ARG_N(1, __VA_ARGS__, _))))
#define BT_MESH_MODEL_OP_VND(b0, cid) ((b0), (cid), ((cid) >> 8))

If we'd want to keep just a single definition per opcode, we might also want to create a helper for the opcode handler lists creation, something like:

#define BT_MESH_MODEL_OP(_op_bytes, _len, _func)                                                  \
	{                                                                                         \
		.opcode = BT_MESH_MODEL_OPCODE(_op_bytes),                                        \
		.len = _len,                                                                      \
		.func = _func,                                                                    \
	}

The other alternative, which has the same overhead as your current solution of having two definitions per opcode, would be to have the "raw" array format as well as the the traditional integer format, i.e. something like:

#define OP_HEARTBEAT_PUB_STATUS_RAW        BT_MESH_MODEL_OP_STD(0x06)
#define OP_HEARTBEAT_PUB_STATUS            BT_MESH_MODEL_OPCODE(OP_HEARTBEAT_PUB_STATUS_RAW)
#define OP_APP_KEY_DEL_RAW                 BT_MESH_MODEL_OP_STD(0x80, 0x00)
#define OP_APP_KEY_DEL                     BT_MESH_MODEL_OPCODE(OP_APP_KEY_DEL_RAW)

Anyway, I just wanted to put the above out there so that it can be considered. Ultimately, this PR is about an optimisation, however since it comes with API changes I want us to acknowledge all possible options before locking us into one (the feature freeze for Zephyr 3.6 is end of next week, so if this gets merged before that then we'll be stuck with it for some time).

[LingaoM]

I'm not saying should go this route, but it's good to at least understand what it'd in practice mean. With the following, the idea is that the opcodes are defined using either BT_MESH_MODEL_OP_STD() (for standard opcodes) or BT_MESH_MODEL_OP_VND() (for vendor opcodes).

That great, cool, thanks!
AKF i'd use BT_MESH_MODEL_OP_SIG()(for SIG Model opcodes) or BT_MESH_MODEL_OP_VND()(for Vendor Model opcodes).

[jhedberg]

I'm not sure I'd make the BT_MESH_MODEL_BUF_INIT() API require the existence of a define whose name must follow the pattern OP_*_RAW. We had to have this kind of required naming scheme with the earlier _LEN solution, but I don't think there's any strong reason to require it now. Someone could be perfectly happy to define their opcodes using only the new _SIG() or _VND() macros, and then use the BT_MESH_MODEL_OPCODE() conversion explicitly in the places where it's needed.

[LingaoM]

So, how todo ?
Change to directly use OP_*_RAW?

[jhedberg]

Yes, the BT_MESH_MODEL_BUF_INIT() would take as input a value created using the new OP_STD() or OP_VND() macros, which in your current proposal is the *_RAW defines.

[jhedberg]

@LingaoM looks pretty good, thanks for adopting my suggestion! :)

I left one more inline comment regarding the requirement of always having *_RAW defines. I think it would also be good to get some more thoughts (especially from @PavelVPV) regarding the "two defines per opcode" solution. Previously, with your *_LEN solution, there was no way to avoid two separate defines per opcode, however now we have the option of just one, if we want to. The downside is that you'd need to wrap this single define through BT_MESH_MODEL_OPCODE() whenever the mesh API requires a uint32_t rather than the raw array. Side note: I think it's good to keep the uint32_t format internally and not change those APIs, since it's a bit more efficient to do message matching by comparing integers instead of byte arrays.

[PavelVPV]

Initially I though it is good that we can get rid of BT_MESH_MODEL_BUF_DEFINE and bt_mesh_model_msg_init pair as I, personally, was always confused that I need to call bt_mesh_model_msg_init() in addition to BT_MESH_MODEL_BUF_DEFINE to actually initialize the opcode in the message buffer because even though BT_MESH_MODEL_BUF_DEFINE takes the opcode as an argument, it only uses it to set length of the buffer. With this change, I'm not sure what improvement we actually get. The rom footprint is increased, slighlty but increased (+96 bytes in the mesh_provisioner sample). Customers will now have to write 2 macros instead of one to define a message opcode. They could use old approach, but do we show how to use it if we change all our code to use 2 macro now? Having 2 APIs is also confusing as I as a customer should now spend time on figuring out the difference and deciding which API should I use. I can not also say that the BT_MESH_MODEL_BUF_INIT is actually user friendly. I think explicitly calling net_buf_simple_add_<u8/le16/be32/etc> was much more readable. The only benefit I see is the length check at the compile time, but was it a problem before? The change looks cool really, but as a customer I don't see how this makes my life easier.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.