RoanBrand / MQTT-Siemens-S7-300

MQTT library block written in Siemens SCL for S7-300 PLC with CP343-1
89 stars 47 forks source link

Mix of MQTT connection state and MQTT connect error codes leads to conflict? #1

Closed CarstenMaul closed 7 years ago

CarstenMaul commented 8 years ago

Hello,

in the function block MQtt within this code block CASE (intState) OF 0 : // Wait for received packet start IF ((Data.runTime - Data.lastInActivity) >= Globals.MQTT_SOCKET_TIMEOUT) THEN Data._state := Globals.MQTT_CONNECTION_TIMEOUT; ELSIF available() THEN myPacketReader.iBegin := true; intState := 1; END_IF; 1 : // Read complete packet IF (myPacketReader.xDone) THEN IF (myPacketReader.result = 4) THEN IF Data.buffer[3] = 0 THEN Data.lastInActivity := Data.runTime; Data.pingOutstanding := false; Data._state := Globals.MQTT_CONNECTED; // here Data._state is used for a defined state. ELSE Data._state := BYTE_TO_INT(Data.buffer[3]); // here Data.state is used to store error codes returned by a connack packet END_IF; // ELSE maybe need equivalent/workaround to client->stop() END_IF; END_IF; END_CASE;

Please look at the Data._state assignmends within CASE (intState) OF = 1 code block. There are two Data._state assignments. The first assignment assigns a defined state Data._state := Globals.MQTT_CONNECTED The second assignment assigns an connack error code to Data._state: Data._state := BYTE_TO_INT(Data.buffer[3])

Data.buffer[3] holds the connack error code returned by a connack mqtt package.

The problem is: within Globals. the state "1" is assigned as Globals.MQTT_CONNECTED within the MQtt docomentation this value is already taken by an connack error code: 0x01 Connection Refused, unacceptable protocol version So if the error returned is 0x01, in my opion the code will set Data._state to Globals.MQTT_CONNECTED, which is wrong state.

Please refer to http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc385349257 (MQtt documentation section 3.2.2.3 "Connect Return code")

RoanBrand commented 8 years ago

Hi

Thanks for the info. I was aware of this at creation, looks like I forgot to fix it. I will commit soon. Thanks for porting to CPU with internal PN! I will want to test it later.

Roan

From: CarstenMaul [mailto:notifications@github.com] Sent: 17 May 2016 10:07 PM To: RoanBrand/MQTT-Siemens-S7-300 MQTT-Siemens-S7-300@noreply.github.com Subject: [RoanBrand/MQTT-Siemens-S7-300] Mix of MQTT connection state and MQTT connect error codes leads to conflict? (#1)

Hello,

in the function block MQtt within this code block CASE (intState) OF 0 : // Wait for received packet start IF ((Data.runTime - Data.lastInActivity) >= Globals.MQTT_SOCKET_TIMEOUT) THEN Data._state := Globals.MQTT_CONNECTION_TIMEOUT; ELSIF available() THEN myPacketReader.iBegin := true; intState := 1; END_IF; 1 : // Read complete packet IF (myPacketReader.xDone) THEN IF (myPacketReader.result = 4) THEN IF Data.buffer[3] = 0 THEN Data.lastInActivity := Data.runTime; Data.pingOutstanding := false; Data._state := Globals.MQTT_CONNECTED; // here Data._state is used for a defined state. ELSE Data._state := BYTE_TO_INT(Data.buffer[3]); // here Data.state is used to store error codes returned by a connack packet END_IF; // ELSE maybe need equivalent/workaround to client->stop() END_IF; END_IF; END_CASE;

Please look at the Data._state assignmends within CASE (intState) OF = 1 code block. There are two Data._state assignments. The first assignment assigns a defined state Data._state := Globals.MQTT_CONNECTED The second assignment assigns an connack error code to Data._state: Data._state := BYTE_TO_INT(Data.buffer[3])

Data.buffer[3] holds the connack error code returned by a connack mqtt package.

The problem is: within Globals. the state "1" is assigned as Globals.MQTT_CONNECTED within the MQtt docomentation this value is already taken by an connack error code: 0x01 Connection Refused, unacceptable protocol version So if the error returned is 0x01, in my opion the code will set Data._state to Globals.MQTT_CONNECTED, which is wrong state.

Please refer to http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc385349257 (MQtt documentation section 3.2.2.3 "Connect Return code")

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHubhttps://github.com/RoanBrand/MQTT-Siemens-S7-300/issues/1 The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorized to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful. Please notify the sender by replying to this message and then delete it from your system. Please report email abuse / misuse to IMQS Software.

CarstenMaul commented 8 years ago

Hello Roan,

I already extended my project, now it works with internal or external PN. Acutally there is some IF-THEN-ELSE stuff to select what ethernet functions to use. Currently I am thinking about two Makefiles and two MQTT FBs, one MQTTPN and one MQTTCP. By doing this it is not mandatory to import all Siemens ethernet stuff (PN FBs 63-66 and CP FBs 5,6,10) into the PLC.

By the way, for the internal PN I used a different method to create the ethernet TCP connection. The connection parameters are configured in a DB (via an UDT). Have a look at it, this approach is more like standard software development works. Unfortunately, this method is not possible with the external CP.

RoanBrand commented 8 years ago

Hi Carsten

I finally got around to fixing the problem.

That extension is great. Your plan of 2 makefiles is better than the alternative of separate projects in folders or 2 repo's. You are more than welcome to contribute and make a pull request if you want to so that this project can work for both cases. I currently do not have a PLC with internal PN with me.

I am aware of the differences in the tcp connection setup between the two. The disadvantage with the CP is that each connection the PLC needs to make must be setup at design time and cannot be modified at runtime.

RoanBrand commented 7 years ago

Great work Carsten. Many thanks for the contribution.

I will start to work again on this soon. As soon as I am comfortable with this code I plan to port to Siemens S7-1200 & S7-1500

You are welcome to contribute directly to this repo any time.