implementing #1594 #1616
implementing #1594 #1616
Conversation
Tried to implement #1594
|
Something to convey to a human that an internal configuration is
inconsistent and a function is being called with an invalid parameter.
Normally, this would be an assert(), but tis project seems to have a LOT of
ASSERT variations and it's hard to tell what's available where.
BT_ASSERT(uuid_value == uuid_value) (or something else you can find to
always evaluate to "true' (not zero)) would display that message and halt
the task in case damage may befall from bad internal status. (e.g. thermal
runaway, though that seems unlikely here...)
…__ASSERT(false, "message goes here"); might be an option.
I think you have most of
https://github.com/Ralim/IronOS/blob/5022eca8af6a03dceac8f4468ee50c1ec719eeaa/source/Core/BSP/Pinecilv2/bl_mcu_sdk/components/ble/ble_stack/common/log.h#L76
at your disposal since you #include log.h
Ideally, assertions tell you what failed and offer hints on fixing it, but
that's kind of hard in an internal configuration case.
You can also do a 'fail-safe" that logs the error and then tries to pick a
sane state so the system will march on and one of those is a generally safe
thing to do
default:
BT_LOG("invalid configuration 0x%0x for foo found" Using default, foo);
// FALLTHROUGH (no break here. Some compilers or languages (e.g.
https://en.cppreference.com/w/cpp/language/attributes/fallthrough) accept a
special comment that yo *intend* do fallthrough)
case brightness:
doBrightness();
break;
Does that help?
On Fri, Mar 10, 2023 at 6:56 PM discip ***@***.***> wrote:
This closes #1594 <#1594>.
@NeilHanlon <https://github.com/NeilHanlon>
@River-Mochi <https://github.com/River-Mochi>
Please let me know if this is working as expected.
@GPSBabelDeveloper <https://github.com/GPSBabelDeveloper>
Unfortunately I had no idea what to put in *default*.
Any suggestions?
------------------------------
You can view, comment on, or merge this pull request online at:
#1616
Commit Summary
- d88eed4
<d88eed4>
Update ble_handlers.cpp
- 6174d80
<6174d80>
Update ble_handlers.cpp
File Changes
(1 file <https://github.com/Ralim/IronOS/pull/1616/files>)
- *M* source/Core/BSP/Pinecilv2/ble_handlers.cpp
<https://github.com/Ralim/IronOS/pull/1616/files#diff-bf1cd2c69c93ac782dfcb49d1d09d6a5ef6bc2c0c7056e58bea5644ad7051ced>
(15)
Patch Links:
- https://github.com/Ralim/IronOS/pull/1616.patch
- https://github.com/Ralim/IronOS/pull/1616.diff
—
Reply to this email directly, view it on GitHub
<#1616>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC3VAD5UKM3NRX2HIURQXVTW3PES7ANCNFSM6AAAAAAVXDOADI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@GPSBabelDeveloper |
|
could just wait for @NeilHanlon since he is also working on the PineSAM BLE app and he wrote the code for this IronOS fix so that these specific things would work correctly from Pinecil V2 communication with the separate PineSAM BLE app. Neil has a full time job during the week, so he probably could not get to these messages until the weekend. He is not gone, I just spoke with him last night on some PineSAM app stuff. he just has a full time job. |
|
@River-Mochi Since the change is marginal (as requested by @NeilHanlon and proposed by @GPSBabelDeveloper: refactoring), the basic function should not be affected. Then again, no one answered, so I just did the obvious. 😊 No offence intended. |
|
Agree w/ @GPSBabelDeveloper re having some sort of assertion which acknowledges the code path is nonstandard and should be reported as a bug. BT_ASSERT seems fine, but I'm not versed in the project well. It's probably worth getting this merged in for 2.21, anyways, though. |
|
@discip no worries. I just thought to wait also for Neil's input might be good idea since we have a some time before Ralim returns. and now we have his feedback. |
discip
requested review from
Ralim and
robertlipe
and removed request for
robertlipe
|
I'm not thrilled with settings actions being directly in the BLE, but you left it better than you found it, so thank you. LGTM |
|
@discip can this be merged? |
|
I would like @Ralim to have look first, as I am not quite sure if he is ok with the way this is done. |
| OLED::setRotation(getSettingValue(SettingsOptions::OrientationMode) & 1); | ||
| break; | ||
| default: | ||
| BT_ASSERT(uuid_value == uuid_value); |
There was a problem hiding this comment.
the intention was to notify that this code path was reached. In general it should mean that this block needs to be revisited (or something went really wrong) - since these are currently the only parameters which require additional handling to apply them without restart.
There was a problem hiding this comment.
I'm not sure why this would be used; this will always evaluate to true and thus not crash the chip.
There was a problem hiding this comment.
I don't disagree. should we crash the chip?
we left it this way to ask you what should be done, basically.
There was a problem hiding this comment.
I think for now we remove this; but I'm going to have to cleanup an "apply" function for settings
|
Yes. There is an internal consistency error indicating invalid internal
state. Invoke the debugger and do not continue.
…On Tue, Mar 28, 2023 at 10:43 AM Neil Hanlon ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In source/Core/BSP/Pinecilv2/ble_handlers.cpp
<#1616 (comment)>:
> OLED::setRotation(getSettingValue(SettingsOptions::OrientationMode) & 1);
+ break;
+ default:
+ BT_ASSERT(uuid_value == uuid_value);
I don't disagree. should we crash the chip?
we left it this way to ask you what should be done, basically.
—
Reply to this email directly, view it on GitHub
<#1616 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC3VAD7FQWXUKYOTFEKYYKLW6MBL5ANCNFSM6AAAAAAVXDOADI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
It seems you are all way ahead of me and I am kind of ashamed for having opened this PR. 😅 So either someone tells me what exactly to change, or I'll close this and someone else (knowledgeable) creates a new PR. 😊 thanks in advance |
This closes #1594.
@NeilHanlon
@River-Mochi
Please let me know if this is working as expected.
@GPSBabelDeveloper
Unfortunately I had no idea what to put in
default.Any suggestions?