philbowles / PangolinMQTT

PangolinMQTT - ArduinoIDE client library for ESP8266, ESP32 and STM32-NUCLEO
Other
71 stars 21 forks source link

connected() LIES #19

Closed leifclaesson closed 4 years ago

leifclaesson commented 4 years ago

It says it's connected when it's not.

It returns PANGO::TCP as a bool. You probably meant to return PANGO::TCP?PANGO::TCP->connected():false;

:-)

philbowles commented 4 years ago

You probably meant to return PANGO::TCP?PANGO::TCP->connected():false;

I didn't. :)

PANGO::TCP is null until TCP connects. On disconnect, it is set back to NULL. Thus its binary status as a bool should correctly reflect the TCP connection status at all times, so you need to explain what you mean by "It says it's connected when it's not." For example what event is suggesting to you that it is not connected? If PANGO:TCP contains a value (and is therefore TRUE) then there is a TCP connection between Pangolin and the MQTT server.

Please clarify providing logs or other evidence

einglis commented 4 years ago

If PANGO:TCP contains a value (and is therefore TRUE) then there is a TCP connection between Pangolin and the MQTT server

That's manifestly not true. You new the connection and later call connect on it. The connect call can fail leaving your state machine in limbo. But that's not even the issue under discussion here.

The logical expectation of calling PangolinMQTT->connected() is to find out if there is an MQTT connection, not whether there is a TCP connection. Since the MQTT ConnectPacket is not dispatched until the TCP connection is established (and even then, as you so often point out, may linger before being 'really' sent), there is a clear window in which connected() can return true, when the MQTT session is anything but.

Edit: although the change suggested by the OP did make my code behave, I realise it was just reducing the window of opportunity for a race condition, so is far from the whole story.

philbowles commented 4 years ago

They are not "protests" that you are doing it all wrong, they are a clear statement of fact. Your code is using the library wrongly. Do not presume to second guess the intention of any api call or my reasons for writing the code the way I chose to.

Also your attempt at rationalising your incorrect statements is also flawed.

  1. I do not implement a "state machine" in any commonly-accpeted meaning of the word

  2. Your assertion re new etc is illogical and incorrect. In between instantiating TCP with new and the connect - NO USER CODE CAN RUN - hence if the "later" (by about 3 or 4 mircoseconds) TCP connect fails, _onDisconnect is called (again without the possibility of any intervening user function being called) and the TCP object is nulled so there is no way the user can ever call anything to see the state of it unless it is validly connected or NULL - hence my statement is correct from the user (to whom it was addressed) point of view. And of course if the TCP connection fails then there can be no MQTT connection - hence TCP being null is functionally identical to there being no MQTT connection.

  3. No change can make your code "behave" , because its is incorrectly structured. Let's just be a tad more adult and listen to what you are saying: "My code is wrong - by my own admission - I have added an unoffical fix (which is in itself in dispute for the same reasons decribed here, until I see any actual evidence to support it) - and now the disputed fix makes my bad code not quite so bad...so I call it "behaving" even though I have already been told I am not using it as per the required documentation and examples and then criticise the author and argue with him...when NO_ONE ELSE HAS THIS PROBLEM and his code CORRECTLY IMPLEMENTS THE MQTT 3.1.1 PROTOCOL". Take a few seconds to think about that, please.

4 "there is a clear window in which connected() can return true, when the MQTT session is anything but" The problem with THAT logic is that there is no point in your "window" at which it makes any sense to call any other Pangolin function until onConnect has fired - so again: and I don't know how many more times I have to say this: if you insist on ignoring a) the documentation b) my advice c) the fact that your code is wrong then you are on your own.

  1. Given that you are polling an async library (and the very nature of this "issue" at all) I suspect you may not fully understand how asynchronous code works on ESP8266 hence I will not be wasting any more time discussing this.

Summary: Your code is wrong, you are using the library wrongly. I can not - and will not - spend time changing a working library for one user's bad code edge-case. Take a deep breath, be man enough to take the advice and stop arguing please.