use the enumeration with an fixed underlying type

[wqx6]

Problem

We define function chip::TLV::TLVWriter::Put with some overloaded functions. The second parameter of the Put function can be uint8_t,uint16_t,uint32_t,uint64_t,int8_t,int16_t,int32_t,int64_t. But in some toolchains (such as riscv32-esp-elf #7057 and arm-none-eabi-gcc), int32_t is defined from long int instead of int, which will cause compile errors when calling Put(uint64_t, enum types).

Change overview

use the enumeration with an fixed underlying type

Testing

the compile success with the toochain riscv32-esp-elf.

[tcarmelveilleux]

This change seems error-prone, and it sounds like the issue is the "too much overloading that can cause unexpected results". Perhaps adding explicit PutInt32, PutUint8, etc, in TLVWriter, where it really matters, would help.

[mrjerryjohns]

Not having overloaded methods, and having explicitly named Put functions as @tcarmelveilleux suggested I don't think actually fixes the problem with enumerations. Fundamentally, you always have to either type-cast the enumeration to a particular stdint type (as is the case in this PR), OR use strongly-typed enumeration definitions as part of code-generation (available since C++11). I'd prefer the latter (and as part of changes we're making to code-gen, I hope to fix this too).

[bzbarsky-apple]

Agreed that having strongly-typed enums would be better. That said:

1. If we have to cast, we should cast to the largest type with the right signedness, so we don't end up with accidental information dropping.
2. For at least some of the callsites in this diff the signedness is wrong. For example, door locks user->type is presumably UserType, which is an enum8 in the spec, which should per spec be encoded as an unsigned integer.

I'm a little curious what the actual compile error here was. Was it failing to pick one of the overloads because none of them really matched better than any others?

[wqx6]

Agreed that having strongly-typed enums would be better. That said:

1. If we have to cast, we should cast to the largest type with the right signedness, so we don't end up with accidental information dropping.
2. For at least some of the callsites in this diff the signedness is wrong. For example, door locks user->type is presumably UserType, which is an enum8 in the spec, which should per spec be encoded as an unsigned integer.

I'm a little curious what the actual compile error here was. Was it failing to pick one of the overloads because none of them really matched better than any others?

@bzbarsky-apple Thanks for suggestion. The compile error was that when compiling with our riscv-esp-elf toolchain, there is not overloaded function Put(uint64_t, int), causing the error of ambiguous call to writer->Put. This was reported in #7057. And some other toolchain(such as arm-none-eabi-gcc) will also have the problem. Considering that the enumerators implicitly convert to int, I think static_cast to int32_t can be a solution.

[bzbarsky-apple]

Considering that the enumerators implicitly convert to int, I think static_cast to int32_t can be a solution.

Sounds like the compiler in this case is catching a pre-existing bug? We should fix that bug, not enforce it via casts...

[bzbarsky-apple]

In particular, we should probably go through the relevant enums and explicitly declare the right storage types for them, with the right signedness to match the spec, instead of doing this casting at the callsites.

[wqx6]

In particular, we should probably go through the relevant enums and explicitly declare the right storage types for them, with the right signedness to match the spec, instead of doing this casting at the callsites.

@bzbarsky-apple I used the enumeration with an fixed underlying type and the compile will not fail. But I don't know whether it can be a solution. Also, I found the definition of TLVWriter::Putoverloaded functions was strange. It seems that we only use the overloaded functions Put(uint64_t,uint64_t) and Put(uint64_t,int64_t).

CHIP_ERROR TLVWriter::Put(uint64_t tag, uint8_t v)
{
      return Put(tag, static_cast<uint64_t>(v));
}

CHIP_ERROR TLVWriter::Put(uint64_t tag, int8_t v)
{
      return Put(tag, static_cast<int64_t>(v));
}

[bzbarsky-apple]

It seems that we only use the overloaded functions Put(uint64_t,uint64_t) and Put(uint64_t,int64_t)

In practice yes, because we do min-size encoding; the resulting TLV just depends on the value passed in and whether it was signed or unsigned, not on the exact C++ type it was represented in.

[github-actions]

Size increase report for "nrfconnect-example-build" from aa96ea0

File	Section	File	VM
chip-lock.elf	text	16	16

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_loc,0,428
text,16,16
.debug_frame,0,-4
.debug_str,0,-4
.debug_ranges,0,-8
.debug_line,0,-19
.debug_abbrev,0,-44
.debug_info,0,-197

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

[gjc13]

LGTM

[gjc13]

@andy31415 PTAL

[dhrishi]

Hi @andy31415 Can this be reviewed and merged? #6345 is blocked on this PR

[bzbarsky-apple]

Note that not properly sizing the enums is leading to the compile issue in #7601 and may well be leading to functional issues too, at least in the places where we are still using the ZCL encoding.

[bzbarsky-apple]

@wqx6 was a followup issue filed to track sizing the enums correctly?

[wqx6]

@wqx6 was a followup issue filed to track sizing the enums correctly?

#7394

[jmeg-sfy]

In particular, we should probably go through the relevant enums and explicitly declare the right storage types for them, with the right signedness to match the spec, instead of doing this casting at the callsites.

Yes this is a problem IMO , i would like to enforce enum width in my cluster declaration to later reuse them in the device code