search

ogabrielinacio / esp_provisioning_ble

A Flutter plugin for provisioning ESP32 modules with Ble
MIT License
10 stars 3 forks source link

Is EstablishSessionStatus.KeyMismatch being returned not only when an incorrect proof of possession is used? #11

Open ivanhercaz opened 4 months ago

ivanhercaz commented 4 months ago

Hi! As I suggest with the title (sorry if it is so long), I think it is possible that when establishSession is used EstablishSessionStatus.KeyMismatch may be returned even for errors that doesn't match with the one it refers.

Looking at esp_prov.dart, I think the catch (e) is catching several errors.

https://github.com/ogabrielinacio/esp_provisioning_ble/blob/e0948a571a4b77b8688e9fe3ae5a04aaa8ea4387/lib/src/esp_prov.dart#L42-L51

In this case I understand it is assumed the next situation:

  1. If there is an error in the try block, it is catched.
  2. Before to return the error, it checks if the application is connected to the device.
  3. If it isn't connected, it throws an EstablishSessionStatus.Disconnected; but if it is connected it assumes it is an error with the proof of possession and returns EstablishSessionStatus.KeyMismatch.

Context

I am testing my application and it connects fine, send and receive messages, etc., everything fine. In another screen, with the device already disconnected, I try to connect to the device using the same flow:

  1. Connect (check if disconnected, just in case, then connects).
  2. Establish session.

But it fails to establish the session and returns EstablishSessionStatus.KeyMismatch. I also received an GATT_INVALID_PDU (4), specifically the next error:

E/[FBP-Android](17973): [FBP] onCharacteristicWrite:
E/[FBP-Android](17973): [FBP]   chr: 021aff51-0382-4aea-bff4-6b3f1c5adfb4
E/[FBP-Android](17973): [FBP]   status: GATT_INVALID_PDU (4)
I/gralloc4(17973): @set_metadata: update dataspace from GM (0x00000000 -> 0x08010000)
E/flutter (17973): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: FlutterBluePlusException | writeCharacteristic | android-code: 4 | GATT_INVALID_PDU

A GATT_INVALID_PDU, if I am not wrong, may be caused for the buffer size, a wrong MTU or even trying to connect multiple times (issue that in this specific case I am reporting I already discard).

Debugging

I am trying to debug a bit better the error, but at this moment I just edited the esp_prov.dart and introduce a logger before to returns the EstablishSessionStatus.KeyMistmatch and, although it is just a small clue, it is interesting what it is returned when I entered a wrong password intentionally and when I introduce the good one.

I log the error with:

logger.w('[ESP PROV] => $e');

Intentionally wrong

This is the case in which the error EstablishSessionStatus.KeyMistmatch must be returned. The log line is:

I/flutter (17973): │ ⚠️ [ESP PROV] => type 'Null' is not a subtype of type 'FutureOr<Uint8List>'

Then returns the EstablishSessionStatus.KeyMistmatch.

With correct proof of possession

This is the case in which the error EstablishSessionStatus.KeyMistmatch must not be returned and may be we can work in returning the specific one or maybe some generic error (¿EstablishSessionStatus.UnexpectedState?). The log line is:

I/flutter (17509): │ ⚠️ [ESP PROV] => Exception: Unexpected state

I could try to research this a bit more or, if you know the solution, you can open a PR and mentioned me to test your branch or a specific commit.

About the errors, I am going to verify if the error I am getting is related with the MTU, because in that case I think there should be two more values implemented in EstablishSession:

Implementing this, we could return the correct one in each case and use the KeyMistmatch just when it is an incorrect proof of possession.

What do you think about it?

Excuse me this long issue! Thanks in advance for reading it! ❤️

P.S. If you consider a better title for the issue, change it without problems! P.S. (2) Above the issue I was having, I wrote an offtopic with the solution in a comment below (https://github.com/ogabrielinacio/esp_provisioning_ble/issues/11#issuecomment-1973310922), but it is also interesting because I think it supports the idea of having more possible EstablishSessionStatus.

ivanhercaz commented 4 months ago 
I/flutter (21478): │ ⚠️ [ESP PROV] => Exception: Empty response

This exception also returns a KeyMistmatch. It happens when MTU is increased over 512. I am using flutter_blue_plus and 512 is the default MTU in the connect() method, although I think it isn't a specific issue of that package and it may happen with any other Bluetooth plugin.

ivanhercaz commented 4 months ago

Offtopic, but interesting. I debug the error I was having and, again, as it happened to me when I reported 9#issuecomment-1951453938 but with a different case, it seems the connection was alive or something was blocking due to misbehavior of the state manager I built with Riverpod.

In the first connection I start a provider to manage the EspProv instance, when all the actions have finish, it disconnects from the device using the same EspProv instance that I started. However, I think something, I have not clearly what, maybe something related with my TransportBle, crashs the other establishSession method. It is a bit weird and I will try to debug better, but just as a note in case someone try to manage this package using Riverpod: when you disconnect the device, invalidate the provider with ref.invalidate(nameOfTheProvider), then, when you start to watch or read another one, you will have a clean instance. In my case, as I also manage the TransportBle with a Riverpod provider, but I just use it as part of my provider for EspProv, when I invlidate the one for EspProv, I also invalidate the one for TransportBle.

I wish to have time to prepare an example using Riverpod, because I think it can be very useful for the community and, of course, for me, because I will learn more ❤️ .

Offtopic finished... Excuse me! Please, keep this issue open to talk about the possible improvement of include more errors. If you want, I can open a PR, but first I would like to know if you, @ogabrielinacio, think it has sense. But, as I indicate in the main issue and in the above offtopic, it is a case in which there isn't a KeyMismatch error but it throws one because it catches all errors as it was one if the device is connect.

ogabrielinacio commented 4 months ago

@ivanhercaz In fact, this part of KeyMismatch is not so correct, I apologize for that. As soon as possible I will try to look into this. If you have time, feel free to do the PR

I'm also sorry I'm not paying as much attention to the project, I'm in a very complicated semester at college, so with work and college and studies it's very difficult to keep up with everything.

ivanhercaz commented 4 months ago

@ivanhercaz In fact, this part of KeyMismatch is not so correct, I apologize for that. As soon as possible I will try to look into this. If you have time, feel free to do the PR

Please, don't apologize for that! It is impossible to catch everything, even if you will dedicate to this work being payed, that I know it isn't the case.

I will try to do a PR with a basic proposal. I will notify you when I open it but review it when you can. Feel free to assign me this issue! Also for the Flutter Blue Plus issue.

I'm also sorry I'm not paying as much attention to the project, I'm in a very complicated semester at college, so with work and college and studies it's very difficult to keep up with everything.

Life is complicated and has a lot of stuff we have to take care. Please, don't beat yourself for not having the necessary time to dedicate to this project. You already done an awesome work developing this awesome package and I am sure I am not the only one who thanks your contribution. You already done the big work and it is very well done ❤️ .

Please, dedicate the time you have without pressure. I wish you the best with your work, with your studies and with your health. A big hug from the Macaronesia!