Allow override ARRAY_CAPACITY in variable array

[PetervdPerk-NXP]

Allow override ARRAY_CAPACITY in variable array to optimize publisher memory usage at compile time.

As a publisher you might already know the maximum size of variable length array. This commit allow to override the XXXX_ARRAY_CAPACITY_ define for variable length arrays.

A good example would be the reg_drone_service_battery_Status_0_2_cell_voltages_ARRAY_CAPACITY_ define. At default the maximum is 255, but for instance your BMS only supports atmost 6 cells.

By default the reg.drone.service.battery.Status_0.2 struct is 1056 bytes

hovergames@hovergames:~/uavcan/nunavut_redefine_test$ ./nunavut_redefine_test 
Size of reg_drone_service_battery_Status_0_2 1056

By allowing to overrule the cell_voltages array capacity we can decrease the struct size tremendously.

#include <stdio.h>

#define reg_drone_service_battery_Status_0_2_cell_voltages_ARRAY_CAPACITY_ 6

#include "inc/UAVCAN/reg/drone/service/battery/Status_0_2.h"

int main(int argc, char *argv[]) {
    reg_drone_service_battery_Status_0_2 msg;
    printf("Size of reg_drone_service_battery_Status_0_2 %li\n", sizeof(msg));
}

We only need 56 bytes now to allocate this struct on our stack

hovergames@hovergames:~/uavcan/nunavut_redefine_test$ ./nunavut_redefine_test 
Size of reg_drone_service_battery_Status_0_2 56

[pavel-kirienko]

This is nice, thanks Peter.

Note that in the serialization function we have a check that the target buffer is large enough:

static inline int8_t reg_drone_service_battery_Status_0_2_serialize_(
    const reg_drone_service_battery_Status_0_2* const obj, uint8_t* const buffer,  size_t* const inout_buffer_size_bytes)
{
    if ((obj == NULL) || (buffer == NULL) || (inout_buffer_size_bytes == NULL))
    {
        return -NUNAVUT_ERROR_INVALID_ARGUMENT;
    }

    const size_t capacity_bytes = *inout_buffer_size_bytes;
    if ((8U * (size_t) capacity_bytes) < 4272UL)
    {
        return -NUNAVUT_ERROR_SERIALIZATION_BUFFER_TOO_SMALL;
    }

So if you pass a smaller buffer here with the aim of conserving memory it will error out. Still, your change makes sense because even if you keep the original buffer size you can still save on the memory footprint of the struct itself. Do you want to keep this behavior unchanged or should we consider a way of disabling the serialization buffer size check? Perhaps a new code generation option would help?

Another question is that of MISRA/AUTOSAR compliance. We take great care to keep violations to the minimum which is why the generated code is virtually devoid of conditional compilation statements, but your added #ifdef breaks that. A possible way to address this is to add another code generation option, which could be actually merged with the previously mentioned one. Such that if the option is enabled, the buffer size checks are not emitted and the ability to override the array capacity is supported. Do you think it makes sense?

[PetervdPerk-NXP]

Indeed we could apply this to the serialization output buffer to save memory there as well, but then indeed we've to change the checks as well.

But we don't have to disable the serialization buffer check entirely right, we could make it a define that recalculates the value based on other defines? Or is this problematic.

So the correct way would be adding a new code generation option that enables

1. #ifndef option to override CAPACITY_BYTES
2. Changed checks of capacity bytes
3. Note that we break MISRA compliance.

[pavel-kirienko]

But we don't have to disable the serialization buffer check entirely right, we could make it a define that recalculates the value based on other defines? Or is this problematic.

It's much harder than it may seem at first, I wouldn't venture there. The serialization buffer size and the footprint of your C structure are completely unrelated, because one is defined by the DSDL memory model and the other comes from the C memory model. You would have to trace back your array capacity changes back to the DSDL model and then recompute the size from that.

[PetervdPerk-NXP]

Indeed you're right it can become very complex due to bit padding.

I've added enable_override_variable_array_capacity option in nunavut where you can override the define and the capacity bytes check is then removed. I'm not that familiar with MISRA but we might want to add the specific rules we're violating when this option is enabled.

Furthermore I've added a check when you override the variable array length you can't make it greater then the maximum specified size in the DSDL definition.

[pavel-kirienko]

Not bad. Let's extend the verification suite to cover your changes and I think we're good to merge, assuming that @thirtytwobits has no objections. He was considering addressing this issue differently by providing an option to use dynamic storage but I don't think it invalidates this approach.

Extending the test suite is easy but a bit tedious:

• Mirror your new option in Findnnvg.cmake: https://github.com/UAVCAN/nunavut/blob/b85dc862ba8c4ff0a3b958ea993a7ba028b411d0/verification/cmake/modules/Findnnvg.cmake#L35-L44

• Propagate it in verification/CMakeLists.txt: https://github.com/UAVCAN/nunavut/blob/main/verification/CMakeLists.txt#L144-L184

• Mirror it in verify.py as well: https://github.com/UAVCAN/nunavut/blob/main/.buildkite/verify.py#L22-L194

• Add a new line at the end of verify_c_*.sh (both platforms) that invokes the verification suite with your option activated: https://github.com/UAVCAN/nunavut/blob/main/.buildkite/verify_c_amd64.sh#L13

Phew. Maybe in the future, we could just export specific environment variables from verify_c_*.sh and have nnvg consume them at the bottom of the call stack, kind of like Click does.

[pavel-kirienko]

Suggested change

	in serialization routines. This will option will disable serialization buffer checks and add conditional compilation
	in serialization routines. This option will disable serialization buffer checks and add conditional compilation

[pavel-kirienko]

Suggested change

	#error {{ t | full_reference_name }}_{{ f.name }}_ARRAY_CAPACITY_ > {{ f.data_type.capacity }}U
	# error {{ t | full_reference_name }}_{{ f.name }}_ARRAY_CAPACITY_ > {{ f.data_type.capacity }}U

[pavel-kirienko]

The changes I described above would break this test: https://github.com/UAVCAN/nunavut/blob/33864a509eaaa9a9a7cdfe06ff611106f791a894/verification/c/suite/test_serialization.c#L462-L467

We probably need to propagate the value of your flag into the generated code. For instance, you could add a new preprocessor definition that resolves to true if your new option is enabled, and check it in the verification suite to conditionally disable this check. Come to think of it, it makes sense to expose all code generation flags, not just the new one -- nunavut/serialization.h is the right place for doing so.

[PetervdPerk-NXP]

The changes I described above would break this test:

I've captured that with this, which doesn't disable the serialization buffer check if you don't override the variable array capacity.
https://github.com/UAVCAN/nunavut/blob/71543663a5ad141b2e25591dccc56a876e5c3d59/src/nunavut/lang/c/templates/definitions.j2#L54-L56

I've added test option definition to test suite as you described above, however we still need to make a separate test where override the variable array capacity and test if it actually works.

Hm I'm breaking CI with this change but not the way how expected it, any idea what this error means?
https://buildkite.com/uavcan/nunavut-pr/builds/321#4dd33132-a1e9-4987-97a4-288ef33f27b6

[pavel-kirienko]

Only @thirtytwobits can save us now.

[PetervdPerk-NXP]

Turns out I had to modify the arguments here below as well

https://github.com/UAVCAN/nunavut/blob/d36b1862cb4a879ae0ad048575ffc56c5e01412a/verification/cmake/modules/Findnnvg.cmake#L50-L56

CI is now successful, but I need to make a separate test to see if override is working correctly.
Is there a stable message I could use for that or doesn't it matter and I can just use reg.drone.service.battery.Status_0.2 @pavel-kirienko ?

[pavel-kirienko]

This is great, thanks Peter.

Re testing, you can just copy-paste that Status message (or define something similar) somwhere to https://github.com/UAVCAN/nunavut/tree/main/verification/nunavut_test_types/test0/regulated and use that.

[PetervdPerk-NXP]

Okay I've made a simple test case, is this okay or did I miss some test cases?
https://github.com/UAVCAN/nunavut/blob/8e8b9eeacc51e6414015247702a730ab028c0182/verification/c/suite/test_override_variable_array_capacity.c#L17-L32

[pavel-kirienko]

We should also test that reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK is handled properly. Other than that it should be good.

[pavel-kirienko]

Suggested change

	#elif !defined(reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK)
	# define reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK
	#elif !defined(reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK_)
	# define reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK_

By convention documented in the header comment, we add a single underscore at the end to reduce the chance of conflict with DSDL entities (what would happen if you have a constant named DISABLE_SERIALIZATION_BUFFER_CHECK in your DSDL)

[thirtytwobits]

I'm having trouble with this change: why do we have reg_drone_service_battery_Status_0_2_ anything in the templates? This is a single datatype in one UAVCAN profile. We can't pollute these templates with specific knowledge.

[pavel-kirienko]

The objective is to disable the check for this type only, keeping this on for other types. This is supposed to be safer than disabling the check for all generated code completely.

[thirtytwobits]

I understand the feature (although using a variable-length array type is a more elegant solution) but the implementation seems unacceptable. Creating a single override for all arrays breaks the design of Nunavut and the need to add a single "battery status" ifdef demonstrates why. The two ways forward I see are:

1. Simply allow for a variable-length array in the types. Your implementation can either provide this or can provide a fixed-length array that is always large enough because of your special knowledge of this type and your battery. This should be a trivial change.
2. Define an overlay feature where you can define DSDL type overlay files that allow you to override implementation details for specific fields in specific types. This is a major change.

[PetervdPerk-NXP]

You're right it applies to all variable length array's in all DSDL files but in the following manner.

/// Array metadata for: saturated uint64[<=3] a_u64
#ifndef regulated_basics_PrimitiveArrayVariable_0_1_a_u64_ARRAY_CAPACITY_
#define regulated_basics_PrimitiveArrayVariable_0_1_a_u64_ARRAY_CAPACITY_           3U
#elif !defined(reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK)
#  define reg_drone_service_battery_Status_0_2_DISABLE_SERIALIZATION_BUFFER_CHECK
#endif
#if regulated_basics_PrimitiveArrayVariable_0_1_a_u64_ARRAY_CAPACITY_ > 3U
#  error regulated_basics_PrimitiveArrayVariable_0_1_a_u64_ARRAY_CAPACITY_ > 3U
#endif
#define regulated_basics_PrimitiveArrayVariable_0_1_a_u64_ARRAY_IS_VARIABLE_LENGTH_ true

If there's a cleaner way I'm happy to adapt to that

Simply allow for a variable-length array in the types. Your implementation can either provide this or can provide a fixed-length array that is always large enough because of your special knowledge of this type and your battery. This should be a trivial change.

Would this mean changing the array allocation from uint8_t array[255] to uint8_t * array, or do you have another idea?

[thirtytwobits]

Would this mean changing the array allocation from uint8_t array[255] to uint8_t * array, or do you have another idea?

Yeah. Here, let's discuss better options here -> #191

[pavel-kirienko]

Pending further discussion, I would like to register my opinion about this PR. Ultimately the decision is up to Scott.

[thirtytwobits]

I'm not super-thrilled with this approach but it is pragmatic. My main concern is that, when we do add a variable-length data structure, this becomes a vestigial feature that we need a major revision to purge.
Regardless, I approve.

[pavel-kirienko]

@PetervdPerk-NXP the CD is going to explode now because we forgot to bump the version number. Can you please do that now?