GCC: MSP430F449: Fix pxPortInitialiseStack using the wrong register for pvParameters

[Forty-Bot]

According to the MSP430 EABI section 3.3,

Arguments are assigned, in declared order, to the first available register single, pair, or quad from the following list into which it fits (with the following special exceptions).

For MSP430 and MSP430X, the argument registers are: R12, R13, R14, R15

Therefore, pvParameters should be passed in R12, as it is the first argument, not R15.

[jefftenney]

For perspective, the MSP430F449 port was written for the old "mspgcc" compiler, which used an unofficial EABI. Since ?2015, TI has recommended the newer "MSP430 gcc" compiler and have included it in Code Composer Studio. It follows the official EABI. This PR seems to be dropping support for the old "mspgcc" in favor or support for the newer "MSP430 gcc". The other EABI changes should be considered too.

(from https://www.ti.com/lit/ug/slau646f/slau646f.pdf)

[Forty-Bot]

Ah, thanks. I didn't see anything in the EABI document about the old ABI.

I think this is the only area affected by the ABI change, since this is the only place we "call" a function manually. I don't believe there are any places we check for return values.

[jefftenney]

I wonder if anyone would ever try to update FreeRTOS in an old project based on mspgcc. They would experience this PR as a breaking change -- and a fairly subtle one. Is there an easy way to support both compilers? I'm thinking of something like a predefined symbol we could use to distinguish mspgcc from the modern gcc.

[Forty-Bot]

OK, how about

#if defined(__TI_EABI__) && !__TI_EABI__
/* old ABI *
#else
/* new ABI */
#endif

ref: slau132y section 2.5.1 and slau132j section 4.8.6

[jefftenney]

No, that's for the TI compiler and assembler. What we want is to distinguish between two different flavors/generations of gcc.

Here's an overview from slau646f that helps:

[Forty-Bot]

I am really inclined just to remove it, since the last release was over 12 years ago. Maybe we should just require gcc >= 4.9.0, which was the first GCC with MSP430 support.

[jefftenney]

Seems fine to me. Anybody upgrading an MSP430 project that old to use new FreeRTOS should be OK upgrading gcc too. You could go ahead and remove support for the old mspgcc in this PR. Then the FreeRTOS team can decide if they would rather keep support for the old compiler.

For additional context, the old "mspgcc" with the unofficial EABI apparently ended at 4.6.3, in May, 2012. See here https://sourceforge.net/projects/mspgcc/. I downloaded and ran it:

C:\Users\jefft\Downloads\mspgcc-20120406-p20120911\bin> msp430-gcc.exe --version
msp430-gcc.exe (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 patched to 20120502)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[aggarg]

Thank you for the change. I think we can make this change backward compatible by adding a config which defaults to MSPGCC ABI for backward compatibility. We can also and add a check if the config is not correctly set. What do you think about this:

suggestion.patch

Note that I do not have this part and therefore, have not tested it.

[Forty-Bot]

I'm really not a fan of the default being something that's been unsupported (both by the community and by TI) for over 10 years. The defaults should reflect best practices.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b1ee2e6) 93.42% compared to head (4c31a73) 93.42%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #947 +/- ## ======================================= Coverage 93.42% 93.42% ======================================= Files 6 6 Lines 3194 3194 Branches 885 885 ======================================= Hits 2984 2984 Misses 103 103 Partials 107 107 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/947/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/947/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.42% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jefftenney]

There's a lot to like about Gaurav's proposal. It's backward compatible, and it catches configuration errors during startup.

@Forty-Bot based on your comment, the question is which default value to assign configUSE_MSPGCC_ABI.

With a default value of 1, developers of new MSP430 projects are likely to hit the configuration catch during OS startup one time. With a default value of 0, legacy applications upgrading FreeRTOS are likely to hit the configuration catch one time. For MSP430 today, automatic backward compatibility may actually be more valuable than providing the smoothest possible experience for developers of new projects.

[Forty-Bot]

Well, we can just detect if the user has an ancient version of GCC and error out. If we get a bug report, we can add an option for the old ABI. Personally, I'm willing to bet the intersection of

• People who are still using mspgcc
• People who update to the latest FreeRTOS kernel
• People who own an ES449

is zero.

[jefftenney]

Wouldn't the intersection be just these two?

• People who are still using mspgcc
• People who update to the latest FreeRTOS kernel
• ~People who own an ES449~

That's because the MSP430F449 port appears to work on most any MSP430 (non X).

Projects still using mspgcc might be more common than you would think. Many legacy MSP430 codebases contain assembly language, and those developers might resist changing the ABI because it would require changes to their existing assembly language.

Anyway, to completely finish this now and never have to worry about it again, maybe we should use Gaurav's suggestion.

[jefftenney]

I'm thinking of something like a predefined symbol we could use to distinguish mspgcc from the modern gcc.

Turns out the preprocessor symbol __MSPGCC__ is predefined in the old mspgcc. It is not predefined in modern gcc for MSP430. You could use it to distinguish between the two compilers/ABI's. Some other open source code for MSP430 does this, and I verified the symbol being present/missing in the two compilers.

[aggarg]

That is a great find @jefftenney . If we use this symbol, then we do not need the startup checks either. @Forty-Bot Do you want to update the PR or do you want us to?

[Forty-Bot]

I think the CI run for checking the copyright is way too rigid. And it's very annoying since it is only on the demo repo even though it checks the kernel too. It also only triggers on changed files. Someone should just mass-fix all files in freertos so random contributors don't have to fix spacing "problems."

[jefftenney]

@Forty-Bot Since this PR has to get approved again, can you change the comment referencing COFF ABI. The mspgcc compiler didn't use the COFF ABI. Old versions of the TI compiler used a COFF-like ABI, and its parameter passing convention matches the official TI EABI. The mspgcc compiler used an unofficial ABI, which TI labeled as a "community" ABI. Let's have the comments just say "mspgcc ABI" and "TI EABI" or similar.

[Forty-Bot]

Went with "MSP430 EABI" since that's the title of slaa534.

[aggarg]

@Forty-Bot Apologies for the inconvenience you faced in fixing CI checks. Thank you for your suggestions regarding the same. We will look into the ways of improving it.

Meanwhile if you are okay to grant us write access to your fork, we are more than happy to do these changes on your behalf.

[aggarg]

@Forty-Bot Please rebase with the main branch and ensure that all the check pass. Alternatively, add me as a collaborator to your fork and I am happy to do this for you. Thanks.

[aggarg]

@Forty-Bot Please revert your whitespace change in portable/GCC/MSP430F449/portmacro.h. It is not needed.

Please apply the following patch: 0001-Fix-header-check.patch

[Forty-Bot]

This was mandatory to add before, and now I need to remove it? Weren't you supposed to fix this?

[aggarg]

This branch is out-of-date with the base branch

Please rebase with main as I cannot merge without it.

[sonarcloud[bot]]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[aggarg]

@Forty-Bot Thank you for your contribution.