Lugaresi / codesys2-mqtt-library

Backport of http://codesys-mqtt-library.sourceforge.net/
7 stars 8 forks source link

Subscription and mutliple messages in one TCP frame #3

Closed cholokla closed 1 year ago

cholokla commented 2 years ago

Hello, I'm using your library with Wago 750-889 and works great with publishing and simple subscription.

However, I have a problem when I subscribe to the wildcard topic ('wago/outputs/+/set') and many messages are transmitted from Homeassistant almost in the same time (using Mosquitto server). I've analysed tcpdump log and saw that when several messages are stuffed in one frame (Nagle's Algorithm) only the first one is being received by the FB_MQTTClient. I've used "set_tcp_nodelay" in the Mosquitto configuration but that helps only partially - there are still some messages in one frame.

Is there any possibility to support such case?

mqtt_tcp_problem

Regards, robert

huwylphi commented 2 years ago

Hi Robert,

This is very interesting. I was not aware of such use case where one TCP frame could contain multiple MQTT frames. _FBMQTTClient currently uses SysSockRecv() for fetching incoming TCP frames and reads the first byte to get the MQTT control packet type (here it would be 03 for incoming PUBLISH messages from the broker). The (first) MQTT frame length could be easily calculated. If I remember, SysSockRecv() returns the total number of bytes fetched (in your use case, it would includes all MQTT frames). So, I could imagine that it should be possible to compare the number of byte of the first MQTT message with the total length of bytes parsed while reading/handling the PUBLISH message. In case the total number of bytes returned by SysSockRecv() is bigger than the bytes parsed, then it would mean that further MQTT messages might exists. These further bytes would have to be parsed like any other incoming frame. This is just my quick analysis of how we could support the use case you described.

TLDR: I could try to support the use case you described. I'm just not sure how I could easily reproduce such scenario... Maybe we could use some public testing MQTT broker where you would publish (dummy) messages by reproducing such scenario of multiple MQTT messages in one TCP frame?

Regards, Philippe

cholokla commented 2 years ago

Hi Philippe, thank you for your answer :)

Well, maybe the quickest way to reproduce that case is to setup Homeassistant in docker, configure several mqtt switches at one card and try to turn them on at once ;) ha_wago

There are some free apps that can create raw TCP packets though (e.g. https://packetsender.com/) but as I understand you should first create CONNECT and then PUBLISH frame.

I can try to write simple app, that publishes several messages in a row, but can't be sure if the "mutliple frame" will be created.

Greetings, Robert

huwylphi commented 2 years ago

Hi Robert, I can try to setup Homeassistant in docker and configure it like you proposed. I always wanted to try Homeassistant, so now this could be my opportunity... I'm currently a bit busy but hope I'll find some time to work on that interesting use case. Kind regards, Philippe

cholokla commented 2 years ago

Great, these are my first steps using Home Assistant, because I was using Openhab before, but I can try to help.

I've installed complete Hass OS (recommended and quickest way) on my Synology NAS but you can also install it on the VirtualBox: https://www.home-assistant.io/installation/. Nextly you should install MQTT Broker integration (I'm using Mosquitto here).

Then, you can configure mqtt switches in the HA's configuration.yaml (simplest way is using the Visual Studio Code integration), for example:

mqtt:
  switch:
    - name: "Wago Mqtt Output 1"
      state_topic: "wago/outputs/1/get"
      command_topic: "wago/outputs/1/set"
      state_on: "ON"
      state_off: "OFF"
      retain: false
    - name: "Wago Mqtt Output 2"
      state_topic: "wago/outputs/2/get"
      command_topic: "wago/outputs/2/set"
      state_on: "ON"
      state_off: "OFF"
      retain: false
    - name: "Wago Mqtt Output 3"
      state_topic: "wago/outputs/3/get"
      command_topic: "wago/outputs/3/set"
      state_on: "ON"
      state_off: "OFF"
      retain: false
    - name: "Wago Mqtt Output 4"
      state_topic: "wago/outputs/4/get"
      command_topic: "wago/outputs/4/set"
      state_on: "ON"
      state_off: "OFF"
      retain: false

(I'm publishing "get" message from the PLC as soon as the state of the output is changed, but you can also use the optimistic flag in the HA)

and declare subscription in codesys:

mqtt(
    i_sBrokerAddress := '192.168.0.37',
        i_uiPort := 1883,
    i_sUsername := 'xxx',
    i_sPassword := 'xxx',
    i_sSubscribeTopic := 'wago/outputs/+/set',
    i_xPublish := FALSE,
    i_xSubscribe := TRUE,
    i_sClientIdentifier := 'WAGO_SUB',
    i_xCleanSession := FALSE,
    i_tCommTimeout :=  t#10s
);

Last step is to create card on the HA dashboard with all the switch entities and a button in header that can turn on/off all of them.

Regards, robert

huwylphi commented 1 year ago

Hi Robert, just wanted to give you some feedback... Thank you for helping me setting HA up. With your help I could run HA very quickly within a docker container and set it up in order to try to reproduce the use case. See here some screenshot of my test environment: image image

Now I should have a test environment ready :)

Regards, Philippe

huwylphi commented 1 year ago

Hello @cholokla (Robert),

I'm back and could implement the feature for fetching multiple payloads from one MQTT frame :)

I saw that @Lugaresi replaced the internal MemSet() and MemMove() functions by the default one from the SysLibMem library. I used that version too in my forked repository as base for the new implementation. But while integrating that version in my local environment I got MQTT connection issue. The reason is because the SysMemMove() function has the same set of input arguments but not in the same order! So I fix the input arg order everywhere where calling that SysMemMove() function. I'm using a PC200 WAGO controller. I don't know if that SysLibMem library is the same for any PLCs... So maybe, depending on your PLC, this method might cause some issue. If this would be the case, then maybe we should rather use the previous function and avoid that library dependency.

Regarding reading multiple payloads from one MQTT frame, it's working in my local environment, so I've created a pull request in @Lugaresi's repo. But before merging that PR, maybe you could try it and confirm it works with your environment?

For that you could for example pull the new lib (or export file) version 2.8 from my forked repo's master branch.

Thank you in advance and looking forward to hearing from you. Regards, Philippe

Lugaresi commented 1 year ago

Hello Philippe, first of all thank you very much for fixing my dumb mistake, I am focusing on other things at the moment and this project was not high priority for me so I haven't look into it much but was in fact experiencing the connection issues you reported. I am also using it on a Wago controller (750-841) and I believe SysLibMem is provided by CoDeSys since it exposes calls that are part of the underlying operating system. I will also test your new changes, feel free to merge the PR as soon as Robert confirms it works. Thank you again and best regards, Flavio

ddokupil commented 1 year ago

Great to see someone is working on that library. I was trying to run the single message version together with mosquitto addon but am getting no CONNACK response and client drops connection. Did you have such issue? I'm using 750-880. With multi message version I get image which pisses me off as I created an empty project to try your library (in both attempts actually). And why am I getting such an error I have no idea as it uses almost no resources: image

Lugaresi commented 1 year ago

Hello, while I am not making use of the multi-message feature it fixed the no CONNACK response issue for me so the PLC happily spams MQTT messages as I want it to. I have never encountered the issue you mentioned, when does it happen?

ddokupil commented 1 year ago

Good to know, there's a chance to overcome CONNACK. Can't really tell why and when "too much data" happens. Mybe it's because of an extra library? It's the only thing changed in comparison with a single message version. Will do some more experiments and get back if I find anything.

huwylphi commented 1 year ago

Hi @ddokupil, thank you for trying out the lib.

  1. when you say "mosquitto addon", you mean you're running the mosquitto MQTT broker, right? (just to be sure we don't mix with the other issue about RabbitMQ). (I'm using the docker container of eclipse-mosquitto v2.0.15)
  2. regarding CONNACK can you confirm you are using the version not yet merged in this repo? just check if you are NOT using the current master version of this repo and try the master version from my forked repo (v2.8) since the PR containing some fix for CONNACK is not yet merged in this repo (we are awaiting for some feedback from @cholokla).
  3. regarding the CodeSys error "too much data for the controller!": how do you consume the lib? by importing the .lib file in the project or by importing the exp file (export of source) in the project?
  4. Do you maybe have other bigger lib imported in the project like for example OSCAT lib? If that's the case, maybe you can exclude single POUs of that lib somehow
  5. Finally can you report your current CodeSys version (I'm using 2.3.9.61)?
Lugaresi commented 1 year ago

You might have some luck messing with the memory area of the controller. On mine I changed it as follows: image (By default it used to be split evenly between memory and retain IIRC) As long as you maintain the size of memory+retain area the same and change the base of the retain area to base of memory + size of memory you should be good. It might still not do anything, for me it seemed to keep away some hard freezes that forced a cold restart of the PLC.

ddokupil commented 1 year ago

@huwylphi

  1. It's a HASS Add-on, mosquitto ver. 2.0.11
  2. precisely this file: https://github.com/huwylphi/codesys2-mqtt-library/blob/feature/support_multiple_payload_in_one_mqtt_frame/mqtt.lib
  3. by importing that lib
  4. Ithat was also my thought, but no. I have nothing else imported image
  5. It's codesys 2.3.9.68 - so slightly newer, I also tried other compiler versions but that makes no diference.

@Lugaresi Alligned with your settings but still no diference.

ddokupil commented 1 year ago

Actually managed to upload the code when created a new project but still havng the CONNACK issue. Is there a way to maybe wait a bit logner for CONNACK?

huwylphi commented 1 year ago

Hi, thanks for these precision. yes you could try to set the input arg _itCommTimeout to e.g. t#5s. For the lib file it should be the correct one. I never tested that HASS add-on but I could try or maybe it probably relies on the v2.0.11 version of the mosquitto broker, so maybe I could also try with an older version v2.0.11 image of the mosquitto docker container image.

huwylphi commented 1 year ago

Hi,

just wanted to check the state...

@ddokupil: did you achieve to connect your wago controller to the MQTT broker? on my side I couldn't test the HA Add-On broker since my HA runs in docker and I think that Add-Ons are not available when running as container... But I could test with the mosquitto broker version 2.0.11 and it worked the same as with v2.0.15. But if you want I can try to start HA OS as VM and add the Add-On broker and try that env. too?

@Lugaresi: thanks for your nice words... Is the connection stable on your side? I'm running that version since a few weeks now without issues.

@cholokla: did you saw the new version and if yes, do you think you could try it in your environment?

Thanks to all for your participation ;)

Kind regards

huwylphi commented 1 year ago

Hi @ddokupil,

Today I could start HA OS in some virtual machine, install the MQTT broker Add-On. Then I could connect my WAGO controller to that MQTT Broker with authentication and subscribe, publish or get incoming publish messages. I could even connect my other docker based HA instance as MQTT client to that MQTT Broker and test the multiple messages in one frame feature (effective topic of this thread). So everything worked as expected on my side. Here some screenshots:

image

image

image

I hope you could or will be able to consume our MQTT lib with your WAGO controller. Feel free to open a new issue in case you have other troubles. Otherwise (@Lugaresi ) if we don't get further feedback from @cholokla in the next days, I would propose to close that issue and merge the new version to the master branch.

Have a nice WE! Philippe

cholokla commented 1 year ago

Hi, I'm sorry I've been gone for so long, but I'm very busy lately. Actually, I'm just building a house where Wago and Home Assistant will be used together :)

Just uploaded corrected library to the PLC, and... Yippee! it seems to be working!!!

There is one more thing I want to mention. Isn't it a bug where you increment send index using a username variable length instead of a password one?

( Password ) IF i_sPassword <> '' THEN iSendIndexCount := iSendIndexCount + 1; abOutBuffer[iSendIndexCount] := INT_TO_BYTE(SHR(LEN(i_sPassword), 8)); ( Password length MSB ) iSendIndexCount := iSendIndexCount + 1; abOutBuffer[iSendIndexCount] := INT_TO_BYTE(LEN(i_sPassword)); ( Password length LSB ) iSendIndexCount := iSendIndexCount + 1; SysMemMove(ADR(abOutBuffer[iSendIndexCount]), ADR(i_sPassword), LEN(i_sPassword)); iSendIndexCount := iSendIndexCount + LEN(i_sUsername) - 1; END_IF

Thank you very much guys for instant feedback, active support and time spent to correct my problem <3 You are great!!!

Regards, Robert

Lugaresi commented 1 year ago

Hello Philippe, hello Robert, Thank you for the feedback, I have not tested it extensively but since it seems to work for everyone involved I agree we should proceed with the merge, as for the issue with the password length Robert mentioned, it definitely looks like an oopsie (line 1020 in your last revision's mqtt.exp), I suppose it can be fixed in a commit following the merge if amending the pull request is extra hassle. Thanks everyone for your involvement Flavio

huwylphi commented 1 year ago

Hi, Thank you for your feedback too. Robert, I imagine your adventure... I also built a house a few years ago and it was a very intense period. But keep it up, you'll survive this ;) Nice catch regarding the password length! I guess it might fall into a CONNECT issue in case the password length is longer than the username. But at the end, since it is the last field in the frame and the remaining length field including the variable header and payload lengths was calculated correctly, maybe it was still working... Or maybe this is the issue that @ddokupil had... Anyway I'll fix that after merging the first PR like proposed by Flavio Have a nice day Philippe

Lugaresi commented 1 year ago

Hello, I have merged the PR and committed the i_sPassword issue. Feel free to check out the latest revision and run some tests, we can probably close this issue if everyone is satisfied. Regards, Flavio

huwylphi commented 1 year ago

Hi Flavio,

Thank you for merging the PR and fix. I agree to close that issue.

We will see which next feature will come :) In the mean time maybe I will be able to play more with Home Assistant or ev. with the other open issue I opened a long time ago regarding RabbitMQ. Maybe that one is fixed too now...

Sincerely, Philippe

chillymattster commented 1 year ago

Hi all,

I went into the CONNACK problem as well a few weeks ago but didn't manage to further debug or isolate the problem. So great to see the progress here! I will try the current master in the next days and let you know if it works for me using 750-830 and 750-881 in combination with HASS and the mosquitto addon.

Cheers, Matthias

ddokupil commented 1 year ago

Hi, sorry for not replying for some time. Today I downloaded a most recent version of that lib and it worked straight away on mosquitto 2.0.11. It also works fine with HASS Add-On version 6.1.3 which works on the same mosquitto version. One thing is worth mentioning, and maybe even worth putting to README.MD. If you play around with that lib, change parameters like user, password and address you have to reset your PLC for those to get actually reloaded, uploading newer version of code is not enough - especially with online change.

huwylphi commented 1 year ago

Hi @chillymattster, thank you for trying it out in your environment. I think it should work but it's always good to have confirmation by several people.

Hi @ddokupil, thank you too for that confirmation. I'm glad it work for you. Regarding the readme and information for PLC reset needs, I'm not sure it is necessary. Actually if you download online changes of a newer version of your PLC program where you changed the MQTT client connection settings such as user, password or end-point address, most probably you would just need to reset the network socket by disconnecting and reconnecting the MQTT client. This possible by just setting the input variable _ixEnable to FALSE and to TRUE again.