Bug in esp_hf_ag_api.c? (IDFGH-3614)

[howroyd]

IDF release v4.1 in file esp-idf/components/bt/host/bluedroid/api/esp_hf_ag_api.c

Lines:

• 182
• 203
• 223
• 244
• 371
• 391
• 418
• 445
• 472
• 499

All lines read return (status = BT_STATUS_SUCCESS) ? ESP_OK : ESP_FAIL;

Am I right in saying BT_STATUS_SUCCESS is the first enum (i.e. zero), therefore the ternary operator will always evaluate to false, therefore ESP_FAIL?

Should the lines not be return (status == BT_STATUS_SUCCESS) ? ESP_OK : ESP_FAIL;

[ghost]

Hi

This status just indicates that ESP has successfully posted the task to BTC to process.
And BT_STATUS_SUCESS is the default status. There is nothing wrong with it, maybe you should run the hfp_ag example and add some log to verify the situation.

Thanks
Weitianhua

[howroyd]

I have run the example and each of the above functions that are called in the example return ESP_FAIL because you are assigning BTC_STATUS_SUCCESS meaning whatever btc_transfer_context returned is overwritten. Then the ternary operator checks, and since the value held by status is now BT_STATUS_SUCCESS (which is zero) it evaluates to false. There is no comparison operator == at all to flip the logic.
I am 100% sure you're wrong here

[PerMalmberg]

@howroyd Is correct, it should read return (status == BT_STATUS_SUCCESS) ? ESP_OK : ESP_FAIL;

Here's a Compiler Explorer comparing the wrong and correct versions:

https://godbolt.org/z/9KoW8M

[ghost]

@howroyd
Oh.. sorry...probably I should go to the hospital to check my eyes...
It was my fault, I will fix it and sync this to Github ASAP.

[howroyd]

@Wth-Esp no worries, easy mistake to make. Glad I could help.

Interestingly, one of the MISRA rules is to always put the constant first when doing comparisons. That way if you accidentally put = instead of == you get a compile time error for trying to assign to a constant. ie:

return (BT_STATUS_SUCCESS == status) ? ESP_OK : ESP_FAIL;

But the following won't compile:

return (BT_STATUS_SUCCESS = status) ? ESP_OK : ESP_FAIL;

[ghost]

@howroyd Oh! Thank you, I get the tip.

[AxelLin]

@howroyd
Oh.. sorry...probably I should go to the hospital to check my eyes...
It was my fault, I will fix it and sync this to Github ASAP.

The fix is not yet in esp-idf master tree.
No idea why it takes such long time for such trivial fix?

And the v4.1-rc does not include this fix.

[Alvin1Zhang]

@AxelLin Thanks for the notes.

• Sorry about this. Because of an internal problem with our CI checks, master branch has not pushed out fixes
• We're working to deploy master ASAP.

Will update once the fix is available on GitHub.

[Alvin1Zhang]

will update once fix on release/4.1 is available, thanks.