fix(RA Host Portable): git rid of the IAR warnings for packed

[abakosh]

This is a daughter PR from PR#2063

I am using the -e flag, for IAR workbench users it is under :
Options/ C_C++ compiler/ Language 1 / Language conformance / Standard with IAR extensions

TU_ATTR_PACKED is not solving the compiler warnings, I thought it should be enough. However, I am using IAR compiler 9.1, which seems insufficient for these structs. When I use the __packed I am getting rid of the warnings

Here is an example given by IAR, which somehow makes it seems that only __packed is supported.
https://www.iar.com/contentassets/e20cfa542d1a4734aa6b234bea42f11a/example-5.pdf
Another link that led me to use __packed on these structs:
https://www.iar.com/knowledge/support/technical-notes/compiler/accessing-unaligned-data/

[HiFiPhile]

The root problem is not packed support, you can read more in #431

In this case since USB registers are 16bit aligned, there is no need to put TU_ATTR_PACKED at top level (for example stm32_fsdev also has 16bit registers), put TU_ATTR_PACKED in the bitfield struct level will make the purpose more clear.

[abakosh]

@HiFiPhile The workaround is suppressing the warnings, but sometimes you can not use it, for example:
return ((volatile uint16_t*)(RUSB2)+ offsetof(RUSB2_REG_t,PIPE_CTR[num - 1]));
here it is throwing an error because "num" is not a constant. __packed suppress all the warnings, I have tested it, seems to work, I am not sure if we can use the following :
#define TU_ATTR_PACKED __packed
instead of:
#define TU_ATTR_PACKED attribute ((packed))
for the IAR compiler. However, __packed is not supported by IAR compiler for RH850, V850 and MSP430 architecture

[HiFiPhile]

I'm agreed IAR has some problem handling packed struct, behavior of __attribute__ ((packed)) and __packed should be the same, either:

• Emit warning for all packed struct
• Smart enough to know that address is in fact aligned

Could you try to change packed keyword into bitfields level, such as:

typedef struct {
  union {
    volatile uint16_t E; /* (@ 0x00000000) Pipe Transaction Counter Enable Register */

    struct TU_ATTR_PACKED {
      uint16_t : 8;
      volatile uint16_t TRCLR : 1; /* [8..8] Transaction Counter Clear */
      volatile uint16_t TRENB : 1; /* [9..9] Transaction Counter Enable */
      uint16_t : 6;
    } E_b;
  };

  union {
    volatile uint16_t N; /* (@ 0x00000002) Pipe Transaction Counter Register */

    struct TU_ATTR_PACKED {
      volatile uint16_t TRNCNT : 16; /* [15..0] Transaction Counter */
    } N_b;
  };
} RUSB2_PIPE_TR_t; /* Size = 4 (0x4) */

[abakosh]

@HiFiPhile Could you try to change packed keyword into bitfields level?

I did, the warning is still there, but when I use __packed it is suppressing the warnings for sure. I have tested it. The commit in this PR is getting rid of the warnings for sure. The only I am thinking of, if there is a neater way to put it in

[abakosh]

behavior of attribute ((packed)) and __packed should be the same.

I agree with you on this, but the compiler doesn't see it this way from my side. The compiler is not doing anything with this attribute ((packed)), maybe in the newer IAR compiler it is not supported or ignored, I don't know

Edit: @HiFiPhile Alright, I figured it out, It was still showing me the warning because I left TU_ATTR_PACKED also on the higher level struct. Now it looks like it passing the warnings.

[HiFiPhile]

I did, the warning is still there

In my test there is no more warning moving TU_ATTR_PACKED to the bitfield struct level.

But since RUSB2_REG_t heavily use bitfield, I feel like adding TU_ATTR_PACKED everywhere is not elegant.

@hathach what's your opinion?

[hathach]

In my test there is no more warning moving TU_ATTR_PACKED to the bitfield struct level.

Yeah, we should move the packed to bitfield if that fixes the issue. And also having TU_VERIFY_STATIC() to make sure the size of the whole struct is correct as well.

But since RUSB2_REG_t heavily use bitfield, I feel like adding TU_ATTR_PACKED everywhere is not elegant.

I don't really look closed enough at the driver, but declaring it with bit field is not a problem at all. Though if we access different field in consecutive statements, maybe it is better to use bitwise to save |=, &=, other than that a single bitfield access would generate the same bitwise op as we could do manually.

[abakosh]

Okay, I think I figured it out. It is a little bit strange but __packed on the higher struct level suppresses the warnings, but attribute ((packed)) only works on the bit field level, however it needs also to be removed from the higher level struct. To maintain the TU_ATTR_PACKED, I agree with Hathach that maybe it is better to put it on the bit field level and solve the problem. I will rework this PR.

[hathach]

look good, though can we still keep the TU_ATTR_PACKED of the overal srtuct ? this is just to prevent any padding between sub-struct member ?

[abakosh]

look good, though can we still keep the TU_ATTR_PACKED of the overall struct? is this just to prevent any padding between sub-struct member?

For some reason, the compiler will keep the warnings, if I do that. This is why I have removed them

[hathach]

look good, though can we still keep the TU_ATTR_PACKED of the overall struct? is this just to prevent any padding between sub-struct member?

For some reason, the compiler will keep the warnings, if I do that. This is why I have removed them

I see, what I interperte is the bit-field struct is packed -> size = 2, however, since the global struct is not packed, it is techically valid if compiler inserted padding between 2 struct/union e.g to have each 4-byte aligned. Since the global packed throw warnings, would you mind adding the TU_VERIFY_STATIC() on the total struct size just in case future compiler cause issue with this. It is nice to have anyway, I could add those veriy myself if it is not convenient for you atm since the PR is good to merge already.

[abakosh]

look good, though can we still keep the TU_ATTR_PACKED of the overall struct? is this just to prevent any padding between sub-struct member?

For some reason, the compiler will keep the warnings, if I do that. This is why I have removed them

I see, what I interperte is the bit-field struct is packed -> size = 2, however, since the global struct is not packed, it is techically valid if compiler inserted padding between 2 struct/union e.g to have each 4-byte aligned. Since the global packed throw warnings, would you mind adding the TU_VERIFY_STATIC() on the total struct size just in case future compiler cause issue with this. It is nice to have anyway, I could add those veriy myself if it is not convenient for you atm since the PR is good to merge already.

Done

[hathach]

perfect, thank you