Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix]Provide default task data when retrieving all descriptors. #888

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

h2zero
Copy link
Owner

@h2zero h2zero commented Feb 7, 2025

  • Update the descriptor filter when trying again with different UUID sizes.

Fixes #884

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

Hmm, getting a LoadException in a different location now:

x/.pio/libdeps/default/NimBLE-Arduino/src/NimBLERemoteCharacteristic.cpp:71
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gattc.c:2630
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gattc.c:2729
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gattc.c:4761
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_att_clt.c:257
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_att.c:519
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs_hci_evt.c:1165
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs.c:249
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs.c:546
x/.pio/libdeps/default/NimBLE-Arduino/src/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h:530
x/.pio/libdeps/default/NimBLE-Arduino/src/NimBLEDevice.cpp:832
xx/.platformio/packages/[email protected]/components/freertos/port/xtensa/port.c:142

@h2zero
Copy link
Owner Author

h2zero commented Feb 7, 2025

That is very odd, I will have to test this tomorrow. @gkoh any insight would be much appreciated in the mean time.

@thekurtovic
Copy link
Contributor

@gkoh Provide the crash register dump if possible, EXCVADDR can usually give a hint.

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

@gkoh Provide the crash register dump if possible, EXCVADDR can usually give a hint.

Good call.
Looks like the incoming error pointer is NULL, given it's trying to access error->status

D NimBLERemoteService: >> retrieveCharacteristics()
I (3445) NimBLE: GATT procedure initiated: discover all characteristics; 
I (3452) NimBLE: start_handle=1 end_handle=5

D NimBLERemoteService: Characteristic Discovery >> status: 0 handle: 2
D NimBLERemoteService: Characteristic Discovery >> status: 0 handle: 4
D NimBLERemoteService: Characteristic Discovery >> status: 14 handle: -1
D NimBLERemoteService: << Characteristic Discovery
D NimBLERemoteService: << retrieveCharacteristics()
D NimBLERemoteCharacteristic: >> retrieveDescriptors() for characteristic: 0x2a00
I (3654) NimBLE: GATT procedure initiated: discover all descriptors; 
I (3662) NimBLE: chr_val_handle=3 end_handle=5

Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.

Core  0 register dump:
PC      : 0x400d5016  PS      : 0x00060130  A0      : 0x800de578  A1      : 0x3ffc97d0  
A2      : 0x00000000  A3      : 0x3ffc234c  A4      : 0x00000003  A5      : 0x3ffc9844  
A6      : 0x00000000  A7      : 0x00000000  A8      : 0xdf5b0e3d  A9      : 0x00000000  
A10     : 0x00000000  A11     : 0x00000001  A12     : 0x00060920  A13     : 0x00060b23  
A14     : 0x00000001  A15     : 0x00000000  SAR     : 0x00000004  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000008  LBEG    : 0x4000c2e0  LEND    : 0x4000c2f6  LCOUNT  : 0x00000000  


Backtrace: 0x400d5013:0x3ffc97d0 0x400de575:0x3ffc9810 0x400de642:0x3ffc9840 0x400e0886:0x3ffc9880 0x400d80fa:0x3ffc98b0 0x400d7b3d:0x3ffc98f0 0x400e546e:0x3ffc9920 0x400e3065:0x3ffc9950 0x400e3098:0x3ffc9980 0x40081184:0x3ffc99b0 0x400d4202:0x3ffc99e0 0x40096eee:0x3ffc9a10

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

According to the manufacturer data, the target advertising device is an Apple product.

@thekurtovic
Copy link
Contributor

thekurtovic commented Feb 7, 2025

I'm stumped, nullptr shouldn't be possible since there is a statically allocated object which is passed to NimBLERemoteCharacteristic::descriptorDiscCB.

/**
 * Returns a pointer to a GATT error object with the specified fields.  The
 * returned object is statically allocated, so this function is not reentrant.
 * This function should only ever be called by the ble_hs task.
 */
static struct ble_gatt_error *
ble_gattc_error(int status, uint16_t att_handle)
{
    static struct ble_gatt_error error;

    /* For consistency, always indicate a handle of 0 on success. */
    if (status == 0 || status == BLE_HS_EDONE) {
        att_handle = 0;
    }

    error.status = status;
    error.att_handle = att_handle;
    return &error;
}

Do you have a copy of the output from the previous PR just for comparison?

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

I'm stumped, nullptr shouldn't be possible since there is a statically allocated object which is passed to NimBLERemoteCharacteristic::descriptorDiscCB.

WTF 😐

Do you have a copy of the output from the previous PR just for comparison?

I reverted to 2.2.1 to reproduce and get:

D NimBLEScan: >> start: duration=0
I (1156) NimBLE: GAP procedure initiated: discovery; 
I (1157) NimBLE: own_addr_type=0 filter_policy=0 passive=0 limited=0 filter_duplicates=1 
I (1163) NimBLE: duration=forever
I (1167) NimBLE: 

D NimBLEScan: Scan started
D NimBLEScan: << start()
I NimBLEScan: New advertiser: 6d:48:e6:25:61:d4
D NimBLEScanCallbacks: Discovered: Name: , Address: 6d:48:e6:25:61:d4, manufacturer data: 4c0010062f1e44c260ed, txPower: 12
I NimBLEScan: Scan response from: 6d:48:e6:25:61:d4
I (1312) test: Device found
I (1315) test: Manufacturer Data = 4c0010062f1e44c260ed
I (1321) test: Name = 
I NimBLEScan: New advertiser: 55:81:8f:34:3f:63
D NimBLEScanCallbacks: Discovered: Name: , Address: 55:81:8f:34:3f:63, manufacturer data: 4c001005461c992193, txPower: 17
I NimBLEScan: Scan response from: 55:81:8f:34:3f:63
I (1344) test: Device found
I (1347) test: Manufacturer Data = 4c001005461c992193
I (1353) test: Name = 
I NimBLEScan: New advertiser: 5d:cc:60:11:41:75
D NimBLEScanCallbacks: Discovered: Name: , Address: 5d:cc:60:11:41:75, manufacturer data: 4c0010051718c1b1d1, txPower: 12
D NimBLEClient: >> handleGapEvent 
I (2049) NimBLE: GATT procedure initiated: exchange mtu

D NimBLEClient: >> handleGapEvent 
D NimBLEClient: >> handleGapEvent 
I NimBLEClient: mtu update: mtu=255
D NimBLEClientCallbacks: onMTUChange: default
D NimBLEClient: << handleGapEvent
D NimBLEClientCallbacks: onConnect: default
D NimBLEClient: exchangeMTUCb: status=0, mtu=255
D NimBLEClient: << connect()
I (2499) test: Connected
I (2502) NimBLE: GATT procedure initiated: discover all services

D NimBLEClient: Service Discovered >> status: 0 handle: 1
D NimBLEClient: Service Discovered >> status: 0 handle: 6
D NimBLEClient: Service Discovered >> status: 0 handle: 10
D NimBLEClient: Service Discovered >> status: 0 handle: 15
D NimBLEClient: Service Discovered >> status: 0 handle: 20
D NimBLEClient: Service Discovered >> status: 0 handle: 25
D NimBLEClient: Service Discovered >> status: 0 handle: 29
D NimBLEClient: Service Discovered >> status: 0 handle: 35
D NimBLEClient: Service Discovered >> status: 0 handle: 45
D NimBLEClient: Service Discovered >> status: 14 handle: -1
D NimBLEClient: << Service Discovered
D NimBLERemoteService: >> retrieveCharacteristics()
I (3138) NimBLE: GATT procedure initiated: discover all characteristics; 
I (3146) NimBLE: start_handle=1 end_handle=5

D NimBLERemoteService: Characteristic Discovery >> status: 0 handle: 2
D NimBLERemoteService: Characteristic Discovery >> status: 0 handle: 4
D NimBLERemoteService: Characteristic Discovery >> status: 14 handle: -1
D NimBLERemoteService: << Characteristic Discovery
D NimBLERemoteService: << retrieveCharacteristics()
D NimBLERemoteCharacteristic: >> retrieveDescriptors() for characteristic: 0x2a00
I (3348) NimBLE: GATT procedure initiated: discover all descriptors; 
I (3355) NimBLE: chr_val_handle=3 end_handle=5

Guru Meditation Error: Core  1 panic'ed (LoadProhibited). Exception was unhandled.

Core  1 register dump:
PC      : 0x400d4ada  PS      : 0x00060430  A0      : 0x800d35f6  A1      : 0x3ffc6bb0  
A2      : 0x3ffca01c  A3      : 0x00000000  A4      : 0x00000000  A5      : 0x00000003  
A6      : 0x3ffc6be0  A7      : 0x0000000c  A8      : 0x800d4ab6  A9      : 0x3ffc6b80  
A10     : 0x00000000  A11     : 0x386c8ba5  A12     : 0x00000000  A13     : 0x00000000  
A14     : 0x00000000  A15     : 0x3ffc2378  SAR     : 0x00000010  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000008  LBEG    : 0x4000c2e0  LEND    : 0x4000c2f6  LCOUNT  : 0xffffffff  


Backtrace: 0x400d4ad7:0x3ffc6bb0 0x400d35f3:0x3ffc6c00 0x400d1b59:0x3ffc6c30 0x400f564f:0x3ffc6c60 0x40096eee:0x3ffc6c90

The backtrace (consistent with before):

x/.pio/libdeps/default/NimBLE-Arduino/src/NimBLERemoteCharacteristic.cpp:118
x/.pio/libdeps/default/NimBLE-Arduino/src/NimBLEClient.cpp:714
x/src/main.ino:43 (discriminator 8)
xx/.platformio/packages/framework-arduinoespressif32/cores/esp32/main.cpp:50
xx/.platformio/packages/[email protected]/components/freertos/port/xtensa/port.c:142

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

For ease of repro, I've pushed to a public repository:
https://github.com/gkoh/NimBLE-Arduino-884

It's targeted at a M5Stack Core2, but any ESP32 should repro (I'd hope).

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

I added some debug, the error pointer isn't NULL and looks to be valid space:

D NimBLERemoteCharacteristic: error = 0x3ffc2344
Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.

Core  0 register dump:
PC      : 0x400d4080  PS      : 0x00060130  A0      : 0x800db5ce  A1      : 0x3ffc9790  
A2      : 0x00000000  A3      : 0x3ffc2344  A4      : 0x00000003  A5      : 0x3ffc97f4  
A6      : 0x00000000  A7      : 0x00000000  A8      : 0x800d407d  A9      : 0x3ffc9740  
A10     : 0x00000031  A11     : 0x3ffc9a68  A12     : 0x3ffc2344  A13     : 0x00060523  
A14     : 0x00000001  A15     : 0x00000000  SAR     : 0x00000004  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000008  LBEG    : 0x400014fd  LEND    : 0x4000150d  LCOUNT  : 0xfffffff9

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

Usually, if I get weird memory behaviour, I've blown the stack.
So I doubled all the various stack sizes ... same crash.

@h2zero
Copy link
Owner Author

h2zero commented Feb 7, 2025

Thanks @gkoh EXCVADDR is not valid though, It's uninitialized. It's late for me so I will have to look further tomorrow, really appreciate the effort!

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

Thanks @gkoh EXCVADDR is not valid though, It's uninitialized. It's late for me so I will have to look further tomorrow, really appreciate the effort!

No problems!
This isn't urgent, the library is so well versioned that I can stay with a known stable release.

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

I got suspicious of the backtrace, so did ye olde printf() debug and got it down to this:

D NimBLERemoteCharacteristic: error1 = 0x3ffc2344
D NimBLERemoteCharacteristic: error2 = 0x3ffc2344
D NimBLERemoteCharacteristic: error3 = 0x3ffc2344
D NimBLERemoteCharacteristic: filter = 0x0
Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.

with this hacking:

    NIMBLE_LOGD(LOG_TAG, "error1 = %p", error);
    int        rc        = error->status;
    NIMBLE_LOGD(LOG_TAG, "error2 = %p", error);
    auto       filter    = (NimBLEDescriptorFilter*)arg;
    NIMBLE_LOGD(LOG_TAG, "error3 = %p", error);
    NIMBLE_LOGD(LOG_TAG, "filter = %p", filter);
    auto       pTaskData = (NimBLETaskData*)filter->taskData;
    NIMBLE_LOGD(LOG_TAG, "error4 = %p", error);
    const auto pChr      = (NimBLERemoteCharacteristic*)pTaskData->m_pInstance;
    NIMBLE_LOGD(LOG_TAG, "error5 = %p", error);

I guess -Og optimisation is still a bit aggressive. I tried with -O0 but the link fails to fit it all in RAM 🙁

@thekurtovic
Copy link
Contributor

bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* filter) const {
    NIMBLE_LOGD(LOG_TAG, ">> retrieveDescriptors() for characteristic: %s", getUUID().toString().c_str());

    // If this is the last handle then there are no descriptors
    if (getHandle() == getRemoteService()->getEndHandle()) {
        NIMBLE_LOGD(LOG_TAG, "<< retrieveDescriptors(): found 0 descriptors.");
        return true;
    }

    int rc = ble_gattc_disc_all_dscs(getClient()->getConnHandle(),
                                     getHandle(),
                                     getRemoteService()->getEndHandle(),
                                     NimBLERemoteCharacteristic::descriptorDiscCB,
                                     filter);
    if (rc != 0) {
        NIMBLE_LOGE(LOG_TAG, "ble_gattc_disc_all_dscs: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
        return false;
    }

    if (filter == nullptr) {
        NimBLETaskData         taskData(const_cast<NimBLERemoteCharacteristic*>(this));
        NimBLEDescriptorFilter filter{nullptr, nullptr, &taskData};
        NimBLEUtils::taskWait(filter.taskData, BLE_NPL_TIME_FOREVER);
    } else {
        NimBLEUtils::taskWait(filter->taskData, BLE_NPL_TIME_FOREVER);
    }

->

bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* filter) const {
    NIMBLE_LOGD(LOG_TAG, ">> retrieveDescriptors() for characteristic: %s", getUUID().toString().c_str());

    // If this is the last handle then there are no descriptors
    if (getHandle() == getRemoteService()->getEndHandle()) {
        NIMBLE_LOGD(LOG_TAG, "<< retrieveDescriptors(): found 0 descriptors.");
        return true;
    }

    NimBLETaskData         taskData(const_cast<NimBLERemoteCharacteristic*>(this));
    NimBLEDescriptorFilter defaultFilter{nullptr, nullptr, &taskData};
    auto                   pFilter = filter ? filter : &defaultFilter;

    int rc = ble_gattc_disc_all_dscs(getClient()->getConnHandle(),
                                     getHandle(),
                                     getRemoteService()->getEndHandle(),
                                     NimBLERemoteCharacteristic::descriptorDiscCB,
                                     pFilter);
    if (rc != 0) {
        NIMBLE_LOGE(LOG_TAG, "ble_gattc_disc_all_dscs: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
        return false;
    }

    NimBLEUtils::taskWait(pFilter ->taskData, BLE_NPL_TIME_FOREVER);

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

I hacked in @thekurtovic changes to retrieveDescriptors(), still getting a similar crash.
However, backtrace is now different, it's claiming LoadProhibited at this point (the last line) of the patch:

NimBLEUtils::taskWait(pFilter ->taskData, BLE_NPL_TIME_FOREVER);

With additional debug output, neither pFilter nor taskData are NULL or invalid looking.

There's something really weird going on here.

@gkoh
Copy link
Contributor

gkoh commented Feb 7, 2025

Just to eliminate the hardware, I swapped to a different ESP32 and can reproduce.

@thekurtovic
Copy link
Contributor

thekurtovic commented Feb 8, 2025

I tried to test the code I posted above* but can't reproduce your issue.

According to the manufacturer data, the target advertising device is an Apple product.

Do you still get this issue if you try connecting to something else?
i.e. esp32 as a peripheral

Complete function
bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* filter) const {
    NIMBLE_LOGD(LOG_TAG, ">> retrieveDescriptors() for characteristic: %s", getUUID().toString().c_str());

    // If this is the last handle then there are no descriptors
    if (getHandle() == getRemoteService()->getEndHandle()) {
        NIMBLE_LOGD(LOG_TAG, "<< retrieveDescriptors(): found 0 descriptors.");
        return true;
    }

    NimBLETaskData         taskData(const_cast<NimBLERemoteCharacteristic*>(this));
    NimBLEDescriptorFilter defaultFilter{nullptr, nullptr, &taskData};
    auto                   pFilter = filter ? filter : &defaultFilter;

    int rc = ble_gattc_disc_all_dscs(getClient()->getConnHandle(),
                                     getHandle(),
                                     getRemoteService()->getEndHandle(),
                                     NimBLERemoteCharacteristic::descriptorDiscCB,
                                     pFilter);
    if (rc != 0) {
        NIMBLE_LOGE(LOG_TAG, "ble_gattc_disc_all_dscs: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
        return false;
    }

    NimBLEUtils::taskWait(pFilter->taskData, BLE_NPL_TIME_FOREVER);

    rc = ((NimBLETaskData*)pFilter->taskData)->m_flags;
    if (rc != BLE_HS_EDONE) {
        NIMBLE_LOGE(LOG_TAG, "<< retrieveDescriptors(): failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
        return false;
    }

    pFilter->dsc = m_vDescriptors.back();
    NIMBLE_LOGD(LOG_TAG, "<< retrieveDescriptors(): found %d descriptors.", m_vDescriptors.size());
    return true;
} // retrieveDescriptors

*Was too lazy to change the subsequent uses of filter.

@gkoh
Copy link
Contributor

gkoh commented Feb 8, 2025

I tried to test the code I posted above* but can't reproduce your issue.

According to the manufacturer data, the target advertising device is an Apple product.

Do you still get this issue if you try connecting to something else? i.e. esp32 as a peripheral

It only seems to fail with whatever Apple target (which I can't physically identify ... possible not in my house ...)

Complete function

Aha! Thank you for that.
Looks like somewhere between cut'n'paste fragments whatever I hacked up was bad.

With this complete function it does not crash!

@h2zero
Copy link
Owner Author

h2zero commented Feb 8, 2025

Thanks for the help and feedback here, been very busy the last week so I only had time to mock up a fix. Will update and merge this in a few moments.

@h2zero h2zero force-pushed the bugfix/retrieve-descriptors branch from 6427ee0 to 94b7457 Compare February 8, 2025 15:10
@h2zero h2zero changed the title Bugfix: add missing taskData when retrieving all descriptors [Bugfix]Provide default task data when retrieving all descriptors. Feb 8, 2025
* Update the descriptor filter when trying again with different UUID sizes.
@h2zero h2zero force-pushed the bugfix/retrieve-descriptors branch from 94b7457 to cb072c3 Compare February 8, 2025 15:16
@h2zero h2zero merged commit fe5a492 into master Feb 8, 2025
31 checks passed
@h2zero h2zero deleted the bugfix/retrieve-descriptors branch February 8, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird crash with discoverAttributes() in 2.2.1
3 participants