Announcements

Help us improve the Power & Sensing Selection Guide. Share feedback

Tip / Sign in to post questions, reply, level up, and achieve exciting badges. Know more

cross mob
User7804
Level 4
Level 4
Hello Infineon,

there is a (non-critical) error in the XMC1100 Flash library:

while (XMC_FLASH_IsBusy() == true)


the "== true" comparison is wrong, because XMC_FLASH_IsBusy() is defined as

return (bool)(XMC_FLASH_GetStatus() & XMC_FLASH_STATUS_BUSY);


IOW the resulting bit is XMC_FLASH_STATUS_BUSY.

The code works, because the defines for "XMC_FLASH_STATUS_BUSY" and "true" are both "1" by chance.

Nevertheless it is an error to compare bit masks from different definitions.

Use "while (XMC_FLASH_IsBusy())" for correct code.

BTW: Where is the best place to report such an error?

Greetings

Oliver
0 Likes
5 Replies
jferreira
Employee
Employee
10 sign-ins 5 sign-ins First like received
Hi,

Thanks for your feedback.
In C any value different than zero when treated as a boolean is considered 'true' otherwise is considered 'false'. This is why the code works.

We could state it more explicitly:
__STATIC_INLINE bool XMC_FLASH_IsBusy(void)
{
return (bool)((XMC_FLASH_GetStatus() & XMC_FLASH_STATUS_BUSY) != 0);
}


Regards,
Jesus
0 Likes
User7804
Level 4
Level 4
Hi Jesus,

although your code works: Why do you want to make it that complicated? If the bit position moves, you add a shift operation to the assembler code. And the "== true" comparison costs more than a zero/nonzero decision.

What are your reservations against my simpler solution "while (XMC_FLASH_IsBusy())"?

Please consider that the libraries are not for a 3GHz Core i7 with tons of memory but a tiny microcontroller with limited resources.

Oliver

P.S.: Please rethink your explanation " why the code works".
0 Likes
jferreira
Employee
Employee
10 sign-ins 5 sign-ins First like received
Hi,

What is wrong with: "In C true is represented by any numeric value not equal to 0 and false is represented by 0."?

It seems you are worried about the comparisons against true (different than 0) or false (equal to zero). These days the compiler is doing a great job.
The assembly code below is extracted from the listing file generated by GCC.

I do not have anything against your solution, my propossal just emphasize the boolean return value of the function.

Thanks for your feedback.

Regards,
Jesus

1. Your propossal:
1000114c 
:
* Details of function

* This routine is the application entry point. It is invoked by the device startup code.
*/

int main(void)
{
1000114c: b510 push {r4, lr}

/* Placeholder for user application code. The while loop below can be replaced with user application code. */
while(XMC_FLASH_IsBusy() /*== true*/)
1000114e: 2401 movs r4, #1
* XMC_FLASH_GetStatus()\n\n\n
*
*/
__STATIC_INLINE bool XMC_FLASH_IsBusy(void)
{
return (bool)(XMC_FLASH_GetStatus() & XMC_FLASH_STATUS_BUSY);
10001150: f7ff fff4 bl 1000113c
10001154: 4204 tst r4, r0
10001156: d1fb bne.n 10001150

{

}
}
10001158: 2000 movs r0, #0
1000115a: bd10 pop {r4, pc}

2. Current code
1000114c 
:
* Details of function

* This routine is the application entry point. It is invoked by the device startup code.
*/

int main(void)
{
1000114c: b510 push {r4, lr}

/* Placeholder for user application code. The while loop below can be replaced with user application code. */
while(XMC_FLASH_IsBusy() == true)
1000114e: 2401 movs r4, #1
* XMC_FLASH_GetStatus()\n\n\n
*
*/
__STATIC_INLINE bool XMC_FLASH_IsBusy(void)
{
return (bool)(XMC_FLASH_GetStatus() & XMC_FLASH_STATUS_BUSY);
10001150: f7ff fff4 bl 1000113c
10001154: 4204 tst r4, r0
10001156: d1fb bne.n 10001150

{

}
}
10001158: 2000 movs r0, #0
1000115a: bd10 pop {r4, pc}
0 Likes
User7804
Level 4
Level 4
Hi Jesus,

jferreira wrote:
What is wrong with: "In C true is represented by any numeric value not equal to 0 and false is represented by 0."?


it's not wrong but neither the reason why the code works nor related to the problem I reported.

The current library code doesn't even use the simple check for zero/nonzero but an explicit value comparison. And, as I wrote, it only works because the defines for "XMC_FLASH_STATUS_BUSY" and "true" are both "1" by chance.

jferreira wrote:
It seems you are worried about the comparisons against true (different than 0) or false (equal to zero). These days the compiler is doing a great job.
The assembly code below is extracted from the listing file generated by GCC.


1. The current code doesn't check for "different than 0" as you write but for "equals one". Therefore the "== true" (equals one) check from the current library tests for a different kind of "true" than the C language definition of "true", this alone is a reason not to use "== true".

2. Regarding cost: If you read my previous post: " If the bit position moves, you add a shift operation to the assembler" you will see the difference. In the current code, the bit mask is 1, therefore no shift is needed.

Oliver
0 Likes
User13264
Level 1
Level 1
I agree with obetz. It is dangerous to encourage comparison to true. The C99 standard specifies that `true` expands by macro to `1` and therefore, in certain cases, would not work as many might expect. http://en.cppreference.com/w/c/types/boolean

However, as jferreia has demonstrated, these two methods compile to the same assembly so something else is probably going on. Compilers are indeed smart these days.

A simple test (with gcc) suggests that casting the value `2` to a `bool` will convert it to `true`. This seems to be what is making this work.

```
#include
#include
int main() {
assert(true == 2); // Fails
return 0;
}
```

```
#include
#include
int main() {
assert(true == (bool)2); // Passes
return 0;
}
```
0 Likes