Pointer return value of a fnc without 'return'

SdkVersion: v3.1-dev-239-g1c3dd23f-dirty
Arduino IDE 1.8.5 with BT sources from esp32-snippets/cpp_utils/BLE*.*
Example:ESP32_BLE_Arduino/examples/BLE_client/

Bug in the methods: getService() and getCharacteristic() in BLEClient.cpp

calls in BLE_Client:

// Obtain a reference to the service we are after in the remote BLE server.
  pRemoteService = pClient->getService(serviceUUID);
  if (pRemoteService == nullptr) // never zero <<<<<<<<<<<<<<<<<<<<<<<<<
  {
    logmsg = "Failed to find our service UUID ";
  }

and:

// Obtain a reference to the characteristic in the service of the remote BLE server.
  pRemoteCharacteristic= pRemoteService->getCharacteristic(charUUID);
  if (pRemoteCharacteristic == nullptr) // never zero <<<<<<<<<<<<<<<<<<<<<<<<<
  {
    logmsg = "Failed to find our characteristic UUID ";
  }

Undepending if the requested Service/Characteristic was found or not, the getService and getCharacteristic returns not 'nullptr'.
In the found case it's the ptr to the Service or Characteristic (thats OK),
in the notFound case it's an uninitialized pointer to mem (not zero or nullptr - NOT OK). The calling fnc reports 'found' and requests the Characteristic from an not existing Service object. The ESP crashes

Fix:
add a return nullptr; to the end of both methods.

method:

BLERemoteService* BLEClient::getService(BLEUUID uuid) {
	ESP_LOGD(LOG_TAG, ">> getService: uuid: %s", uuid.toString().c_str());

	if (!m_haveServices) {
		getServices();
	}
	std::string uuidStr = uuid.toString();
	for (auto &myPair : m_servicesMap) {
		if (myPair.first == uuidStr) {
			ESP_LOGD(LOG_TAG, "<< getService: found the service with uuid: %s", uuid.toString().c_str());
			return myPair.second;
		}
	} // End of each of the services.
	ESP_LOGD(LOG_TAG, "<< getService: not found");
return nullptr; // ++ add this <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
} // getService

Same for getCharacteristic() !

PLEASE CHECK ALL YOUR CODE, THX

-Pit

[nkolban]

Howdy sir ... I checked the code base, to the best of my examination ... the BLEClient::getService() did indeed return a nullptr up until 3 months ago where we changed it throw an exception if the service were not found.

In summary today ->-3 months - Threw exception
-3 months to -8 months - returned a nullptr
-8 months and earlier - no return of a nullptr nor exception ...

Can I ask you to compare your code base against the current and see if there are distinctions?

it's the newest one
SdkVersion: v3.1-dev-239-g1c3dd23f-dirty
Arduino IDE 1.8.5 with BT sources from esp32-snippets/cpp_utils/BLE*.*
Example:ESP32_BLE_Arduino/examples/BLE_client/

it returnes a pice of memory, sometimes it's empty, sometimes not.

It's not a question of 3 months to -8 months , but of coding style!

existing code since a year:
pRemoteService = pClient->getService(serviceUUID);
if (pRemoteService == nullptr)
{
logmsg = "Failed to find our service UUID ";
}

And the Arduino IDE not support the exception handling up to now, or?

[nkolban]

Howdy Sir,
Again ... many, many thanks for taking the time to provide guidance and feedback. I had failed to read the original post correctly and was looking in BLEClient in the core of the libraries instead (as you clearly said) BLE_client in the examples. I do see that the the client is now out of sync with the core code base and what we should now have is a try/catch around the call to getService(). An exception will be thrown if we attempt to get the service and it isn't found. We will correct that and MANY thanks for the indication.

Hi, no problem thank you for the great support! Miss it in the last weeks :-)

Please keep in mind, that your examples are often the base of starting a new development. So segments of api calls and handle this will be still in the users code.

A call of async exception is not a clear exit and end of the function!
So, it's very simple to be compatible to the examples and other user codes be keep or re-add the return nullpointer; at the and of the of getService() and getCharacteristic()in BLEClient.cpp.

Also it's good coding style to return from a functioncall with a clear defined return value. And better: to only return at only one place at the end of the function and store a return value (i.e result or error) down to this point.

bad style (because of multiple returns and no one at the end):

x fnc()
{
 if() return; //not nice: 
 else if ()return; //also not nice here: in a big code segment a single return can be unrecognized by codereview
 //no return; //problem: undefined return value
}

better style:

x fnc()
{
 error=-1;
 if() error = 1;
 else if ()error = 0;
 return error; // we leave the fnc only here
}

(preudo code)

my 5 ct

[nkolban]

Thank you sir. I would consider a function that returns a value and has a path that doesn't return a value to be a serious error ... so serious in fact that I'm surprised a compiler would even allow it. If those crept it then I apologize and there is not even the beginnings of an attempt to defend that.

As for try/catch vs return codes ... oh boy ... we'll get into red vs blue on that one and I'd like to avoid it. If we google on "exceptions vs return codes" there is a world of heated discussion. With that in mind, I'm not going to try and defend the use of "exceptions vs try/catch" ... instead as the current steward of this project which has lots of committers (thanks to all) ... I'll go with what the community want. If the desire is exclusively return codes, we'll go that way. If the desire is exclusively exceptions, we'll go that way. My goal is to provide service to the community and if I can't convincingly articulate why A is better than B or B is better than A, I'll let my betters or consumers decide and fall in-line behind that.

As for absence ... I'm far from gone on this project ... however I've been devoted to a specific project (not related to this one) since the end of December and will be through the end of April. Since all these projects are spare time endevours, there will be periods of fervent attention and periods of no apparent love. That said, I'm always still here and if an emergency comes up and something stops working (that previously worked) or a war breaks out, I'll switch priorities to do what I can.

[Phando]

Is there a fix for the panic reboot on getService or an update on what the sample code should look like?

My chips can find the service but when they get to the connect line, they reboot.

// Obtain a reference to the service we are after in the remote BLE server. BLERemoteService* pRemoteService = pClient->getService(serviceUUID);

The code above produces the trace below.

`
Found our device! address: Forming a connection to 44:fd:76:18:b3:4b

• Created client
  �[0;31mE (972) BT: btc_gattc_call_handler()�[0m
• Connected to server
  �[0;31mE (3471) BT: bta_gattc_cmpl_cback unknown conn_id = 3, ignore data�[0m
  abort() was called at PC 0x4014eac7 on core 1

Backtrace: 0x4008a7f0:0x3ffd8660 0x4008a8ef:0x3ffd8680 0x4014eac7:0x3ffd86a0 0x4014eb0e:0x3ffd86c0 0x4014ebbb:0x3ffd86e0 0x400d45c5:0x3ffd8700 0x400d46cf:0x3ffd8760 0x400d2be2:0x3ffd87b0 0x400d2eb6:0x3ffd8800 0x401692e2:0x3ffd8860

Rebooting...
`

[lociii]

Any news about that issue?
Other projects are trying on this library and are unfortunately in an unusable state right now.

[chegewara]

Hi @lociii , unfortunately Neil is busy with his paying project, also i dont have time now to do anything with library (i have some bugfixes, but this not yet). It would be nice to have some help from community.

You just liked issue from other repository, but there is no info about environment, or i didnt read thorough enough. I can only guessing that issue is caused by this line:
https://github.com/nkolban/esp32-snippets/blob/master/cpp_utils/BLEClient.cpp#L339
and should be easy to fix if you change this line with return nullptr;.

Thats all i can offer now.