STM8 library bug - GPIO_ReadInputPin()

[swhite2]

Posted on June 11, 2010 at 14:45

STM8 library bug - GPIO_ReadInputPin()

[mozra272]

Posted on May 17, 2011 at 15:09

Hi StuartMW,

  The GPIO_ReadInputPin() function use a BitStatus value cast, so when casting the bitmask value with the BitStause type we should have SET or RESET. BitStatus GPIO_ReadInputPin(GPIO_TypeDef* GPIOx, GPIO_Pin_TypeDef GPIO_Pin) {     return ((BitStatus)(GPIOx->IDR & (vu8)GPIO_Pin)); } I don't see any issue in this function... regards mozra

[swhite2]

Posted on May 17, 2011 at 15:09

Using a cast from an unsigned byte to an enumeratied type is not guaranteed to produce the desired result. It may work with some compilers but it does NOT with IAR's EWSTM8. I know because I looked at the value returned  and it was NOT 0/1.

The safe, and IMO proper, way to code this is return ((GPIOx->IDR & (vu8)GPIO_Pin) != 0 ? SET : RESET); That will work with any compiler. BTW if you believe that a cast should work consider this typedef enum             {                 ZERO,                 ONE,                 TWO,                 THREE             } mytype_t; Lets say I have a u8 type variable i which has the value 0x40. What would be the result of ''(mytype_t) i''? Obviously a compiler can't  divine the programmers intent so the result would be undefined.

[Clifford1]

Did you report this bug with ST Support, or only here?

Astonishingly this remains uncorrected at https://documentation.help/STM8S-STM8A/stm8s__gpio_8c_source.html

Suggest:

return (GPIOx->IDR & (vu8)GPIO_Pin) == 0 ? 
RESET :
SET;

rather than the undefined behaviour cast.  

Discussed on StackOverflow at https://stackoverflow.com/questions/77727600/evaluation-if-conditional-statement-on-st-visual-developer-with-cosmic-for-stm8/77729742#77729742 more than 12 years after reporting it here.

In my experience (albeit on STM32) if you report a bug  (and do the analysis for them as you have) it gets fixed.  My experience is also that there are an unfortunate number of bugs, and some rather dubious code and design decisions.

[Clifford1]

On reflection I think the better resolution would be to change the return type to GPIO_Pin_Type  since these are bitmasks, and it is possible to call the function with a combination of bits e.g. GPIO_PIN_4 | GPIO_PIN_3, where a single result SET/RESET would make no sense.

The problem with that is that GPIO_Pin_TypeDef is an enum, so OR'ed combinations result in values that are not defined enumeration values.  Overall it is a rather dubious design.

[Tesla DeLorean]

Cross-linking the duplicate post made here, please people if you post the same question other places, link to those..

https://community.st.com/t5/stm8-mcus/evaluation-if-conditional-statement-on-st-visual-developer-with/td-p/623481

Returning the mask is probably the simplest and most desirable mode as it's less ambiguous about which pin is signalling. Also simple for a 8-bit MCU, most of those set flag bits based on AND operation, and not needing to fit the answer to a specific enumerated number space.

Evidently an issue with Cosmic and IAR implementations, although STM8 is unlikely to get warmed over much at this point.

The joys of using and porting tools and platforms..

[Clifford1]

> Evidently an issue with Cosmic and IAR implementations

What toolchain would this work in?  Neither IAR nor Cosmic are incorrect in this respect.  It is an issue with the library that it invokes undefined behaviour, not the compiler.  It would be an unusual compiler that behaves differently in my experience.

The issue was raised in 2011, and the library was last updated in 2014 AFAIK. Plenty of opportunity to have corrected it.

The name of the function and the documentation suggest it is intended to operate on a single pin, but the implementation would allow combinations.  It would be strange to change the semantics after the event and would possibly break existing code.  Better to add new functions and fix this one to adhere to the intended semantics the broken ones.

> STM8 is unlikely to get warmed over much at this point.

Indeed, but the library is provided as source, so my advice would be to fix it locally and take control of the library code in your own local repo/SCM and maintain it locally.

[Tesla DeLorean]

>>What toolchain would this work in?  Neither IAR nor Cosmic are incorrect in this respect.  

None probably, it's a BLIND CAST into an enumerated number space. Arguably SET is the entire number space outside of zero, so it's the "== SET" that's failing to address everything that's NOT-ZERO

https://documentation.help/STM8S-STM8A/stm8s_8h_source.html#l00232

>>Indeed, but the library is provided as source, so my advice would be to fix it locally..

Yes, that's generally how I've managed errors in ST's SPL

These forms would be preferable, in my opinion, for something that's highly portable

if(GPIO_ReadInputPin(GPIOC,GPIO_PIN_2)) // SET

if(!GPIO_ReadInputPin(GPIOC,GPIO_PIN_2)) // RESET

Although none really address the potential use case (admittedly contrived, but in the spirit of masking, and #defines for register bit flags used elsewhere in ST's sources)

if(GPIO_ReadInputPin(GPIOC,GPIO_PIN_2 | GPIO_PIN_3) == GPIO_PIN_2)

>>The issue was raised in 2011

Well at least 2010, but that date might be artificially later than the original due to the fact we've gone through several forum iterations since 2007-2008. Unfortunately ST hasn't had open bug-tracking systems, or actively used GitHub during most of that period, so there's a lot of these types of things that don't get fixed. Hopefully some Static Analysis tools flag up this stuff.

[Clifford1]

> These forms would be preferable, in my opinion, ...

Indeed that was (included in) my advice to the questioner on StackOverflow as a defensuve coding technique.