Remove hardware dependence in portmacros.h

[mayl]

Remove hardware dependence in portmacros.h

Description

The IAR MSP430X port #include "msp430.h" which pulls all the hardware register definitions into anything which #include "FreeRTOS.h". This removes that hardware dependency "leak" by removing the header file and re-defining the portDISABLE_INTERRUPTS() and portENABLE_INTERRUPTS() macros in terms of __asm.
Test Steps

---

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

I tested in IAR at work, don't have a license here at home to test with. Verified by looking at the disassembly of the portDISABLE_INTERRUPTS() and portENABLE_INTERRUPTS() macros and confirming they are identical to before.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aggarg]

We do the same thing here as well - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/CCS/MSP430X/portmacro.h#L43. Did you face any problem for which you made this change?

[mayl]

Well for one thing, I think conceptually taking a dependence on FreeRTOS.h should not imply taking a dependence on a particular HW implementation. I don't want people who are writing libraries in my projects to start silently leaking hardware specific details into their work without compiler warnings into what could otherwise be pure library code.

Secondly, getting rid of this HW dependency would make this port consistent with the vast majority of other port_macro.h implementations (I obviously did not audit them all). The GCC MSP430 port for instance also uses asm directives instead of #include "msp430.h"

The specific issue that prompted this PR was using a part which has an RTC_C. IAR defines BIN2BCD() and BCD2BIN() macros in msp430.h and I had name conflicts in libraries which we deploy on multiple processors, and don't use the RTC_C or any MSP430 hardware but do use FreeRTOS calls.

Is there hesitation about merging this or are you asking because you would like to adjust the CCS port as well? That would take me a minute because I don't have the CCS toolchain set up, but if you have it set up and can audit the disassembly it's pretty straight forward to confirm that the enable/disable interrupts macros emit the same two instructions before and after the change.

[mayl]

No substantive change in the force-push, just realized I had unbalanced bracket spacing

[aggarg]

I was trying the same change on CCS and figured that inline assembly is not supported for it - https://e2e.ti.com/support/tools/code-composer-studio-group/ccs/f/code-composer-studio-forum/438841/inline-asm-function-for-c-program. So we cannot do it for CCS port.

I found the following when I tested these changes for IAR -

1. These definitions in port.c need msp430.h. So you need to include msp430.h in port.c.
2. You should also change the definition of portNOP:

    #define portNOP()    __asm volatile ( "NOP" )
   

[mayl]

Thanks for taking a look. It's strange that it compiles for me without warning even without the portNOP() and #include "msp430.h" in port.c. I think it has to do with the double underscore functions being compiler intrinsics which are always available. That said I have no objection to the changes you suggest and just pushed them up.

I read the TI e2e post you linked, but did not come to the same conclusion as you. The poster there was interested in manipulating registers across the C/ASM boundary which the TI compiler does not support. Simple insertion of a DINT or EINT or NOP instruction should not have any problem. I looked at the compiler manual and it also suggests the asm directive should work just fine because there is not register access. It's also worth noting that forum post is creeping up on a decade old at this point, so asm support may have changed/improved since then.

Some snippets from the compiler manual (published 2021) that suggest to me this would work in CCS:

[aggarg]

That said I have no objection to the changes you suggest and just pushed them up.

Thank you.

I read the TI e2e post you linked, but did not come to the same conclusion as you.

I did not arrive at this conclusion by just reading the post/manual. I created a project and made the changes and when the compilation failed, I tried to search for the reason which is when I found this.

[mayl]

I don't want to get too hung up on CCS since it's not really the main thrust of this PR, but something still didn't seem right about this... I realized I can access the TI compiler at dev.ti.com so I did some expiriments with CCS there.

Firstly, if you don't have a space before the asm directive like the manual says, you get an assembler error like below:

However, once I added spaces so the assembly was legal I got this to work:

And the disassembly confirming the compiler emitted the correct instructions (I verified with no optimizations and with full optimizations):

Is any of this consistent with what you see in whatever your CCS test environment is? Should I include the CCS port in this PR?

[aggarg]

However, once I added spaces so the assembly was legal I got this to work:

Thank you for pointing this out. This is what I missed.

I tried your changes in CCS and I get a warning saying that "NOP is needed before setting GIE bit". I added a NOP before EINT and the following changes wok for me -

#define portDISABLE_INTERRUPTS()    __asm volatile ( " DINT\n" " NOP" )
#define portENABLE_INTERRUPTS()     __asm volatile ( " NOP\n" " EINT\n" " NOP" )

#define portNOP()                   __asm volatile ( " NOP" )

The above code generates the exact same assembly as compiler intrinsic:

28      	portDISABLE_INTERRUPTS();
0110c8:   C232                DINT    
0110ca:   4303                NOP     

30      	portENABLE_INTERRUPTS();
0110d2:   4303                NOP     
0110d4:   D232                EINT    
0110d6:   4303                NOP     

I also needed to add #include "msp430.h" to port.c. With these changes I can see the correct assembly and build, flash and run the code as well. Would you please include these changes in the same PR?

I also noticed that IAR compiler intrinsic also inserts NOP before EINT:

   \   00001A   0343         NOP
     16            portENABLE_INTERRUPTS();
   \   00001C   32D2         EINT
   \   00001E   0343         NOP

which does not get inserted with the current definition:

   \   000014   91530000     ADD.W   #0x1, 0(SP)
     16            portENABLE_INTERRUPTS();
   \   000018   32D2         EINT
   \   00001A   0343         NOP

Would you please update the portENABLE_INTERRUPTS for IAR also to include a NOP before EINT:

#define portENABLE_INTERRUPTS()     __asm volatile ( "NOP\n" "EINT\n" "NOP" )

Thank your for your contribution!

[aggarg]

@mayl I have made the changes I suggested in my previous comment. Let me know if you have any concern.

Thank you for your contribution!

[mayl]

Hi, thanks for that. Work got busy for me and I haven't been able to look at this.

I tried your changes in CCS and I get a warning saying that "NOP is needed before setting GIE bit". I added a NOP before EINT and the following changes wok for

The other reason for my slow response is that this also seems suspect to me, and I wanted to run it to ground... I found some indications that this error in CCS may be a bug. I looked at how the IAR intrinsics work (not the ASM we wrote here, but the actual intrinsics) and they seem to only insert a nop before EINT if the instruction before was a DINT. This makes some sense because the pipeline would cause interrupts to maybe never be disabled (FreeRTOS handles this case with the NOP after DINT). What's less clear why in a general case you would need a NOP before EINT...

Anyway, I'm not fully sure yet but you may see a PR in the future pulling out the extra NOP when I get my head fully around what's happening in the warning you are responding to...

[aggarg]

I looked at how the IAR intrinsics work (not the ASM we wrote here, but the actual intrinsics) and they seem to only insert a nop before EINT if the instruction before was a DINT.

I did not look at IAR intrinsics but looked at the assembly produced and for me, it inserted a NOP before EINT even when the previous instruction was not DINT.

Anyway, I'm not fully sure yet but you may see a PR in the future pulling out the extra NOP when I get my head fully around what's happening in the warning you are responding to...

Sure. Till then, the code functionally correct and we can merge this PR.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud