pixavier / mqtt4snap

MQTT for Snap!
9 stars 6 forks source link

Get library added as Snap!7 extension #3

Open cymplecy opened 2 years ago

cymplecy commented 2 years ago

Hi Xavier I am a big fan of your MQTT library and use it in several projects. :)

I'd like to see if incorporated as an Snap!7 extension but I've made a few modifications that I'd like to discuss with you :)

I use your original connection callback concept quite a lot so wanted to restore it.

Also, I was working with a broker that required the keepalive time to be set to non-default value so I needed to add that as an optional parameter as well

I've come up with this method to add in the extra two parameters but visually indicating that they are optional image

My idea is to keep the default block uncluttered but allow advanced usage if needed

What do you think?

I am more than willing to put the work in to get the library accepted by Jens as an extension (so that users don't have to enable JavaScript)

regards

Simon

The . at block beginning is just something I do if I duplicate and alter an original library block so that I don't get confused :)

I have a few other modifications as well to propose but this is the main one :)

pixavier commented 2 years ago

Hi Xavier I am a big fan of your MQTT library and use it in several projects. :)

I'd like to see if incorporated as an Snap!7 extension but I've made a few modifications that I'd like to discuss with you :)

I use your original connection callback concept quite a lot so wanted to restore it.

Also, I was working with a broker that required the keepalive time to be set to non-default value so I needed to add that as an optional parameter as well

I've come up with this method to add in the extra two parameters but visually indicating that they are optional image

My idea is to keep the default block uncluttered but allow advanced usage if needed

What do you think?

I am more than willing to put the work in to get the library accepted by Jens as an extension (so that users don't have to enable JavaScript)

regards

Simon

The . at block beginning is just something I do if I duplicate and alter an original library block so that I don't get confused :)

I have a few other modifications as well to propose but this is the main one :)

Hi Simon,

Nice to hear from you.

I suggest using the emqx public broker in the Hello World! example, but you have made me realize that the intermittent problems with test.mosquitto.org come from the keepalive setting.

I agree with all of your proposals. The callback block in connection is a missing feature of MQTT4Snap! current implementation.

To keep backward compatibility, I suggest adding the "connect to" block you propose to the library, which will not conflict with the current "connect" block that could be considered deprecated.

What do you think?

Regards,

XPi

cymplecy commented 2 years ago

image

I think there is an opportunity to leave the old library alone and just use any new or modified ones in a Snap7 extension.

I think having two versions of a block in the same library would look strange.

If someone had an existing 6.9 project and they loaded into Snap!7, everything would work as before. If the new extension/library block looked different - for example, custom category and/or an icon, then there shouldn't be any confusion

It would be up to the programmer to decide to switch to new version whenever they wanted to.

Example image Image edited as I had the topic/payload the wrong way around! :)

pixavier commented 2 years ago

I think there is an opportunity to leave the old library alone and just use any new or modified ones in a Snap7 extension.

Ok

I think having two versions of a block in the same library would look strange. If someone had an existing 6.9 project and they loaded into Snap!7, everything would work as before.

Ok, and if eventually the keepalive setting is needed, the programmer can import the new connect block into the 6.9 project

If the new extension/library block looked different - for example, custom category and/or an icon, then there shouldn't be any confusion

I agree. Custom categories are a great new feature of release 7, and the cloud icon looks nice.

It would be up to the programmer to decide to switch to new version whenever they wanted to.

Ok, idem as said before.

As for me, we can move on.

cymplecy commented 2 years ago

image

Because this would be based on extensions (to avoid need to enable JavaScript) then that wouldn't be possible. Non of the new Snap!7 libraries work in 6.9

As well as 2 extra parameters for connect block, I've added 2 parameters to the publish block - QoS and retain flag

And my last visual changes are to change to being more verbose with the functions of the blocks

image

I did this to make it more friendly and explicit to beginners to MQTT

But if you wish I would return them to the short versions.

pixavier commented 2 years ago

I like the short versions, but in keeping with the general style of the Snap! blocks, the friendly and explicit style seems more appropriate. Ahead with your proposal.

cymplecy commented 2 years ago

Thanks you very much :)

Now the tricky part - learning to make a Snap!7 extension :)

cymplecy commented 2 years ago

I have managed to get it working as an extension and am just talking to Jens about whether mqtt.js can be dynamically loaded as it is now or whether it will have to be directly incorporated into the extension

I have developed this on my own local clone of Snap but will try and get it up to a public server so you can see what I've done and we can discuss some of the details then :)

pixavier commented 2 years ago

Nice! The following bundle is which is used in the current implementation: https://unpkg.com/browse/mqtt@4.2.5/dist/ The 4.2.8 is the last release, but sometimes I've found issues with the test.mosquitto.org broker with the last releases.

cymplecy commented 2 years ago

I'm always getting problems with test.mosquitto.org :(

I'll update to 4.2.5

Which broker do you want to have the blocks defaulting to?

I personally use mqtt.eclipseprojects.io/mqtt:443 for my own testing (as well as own personal brokers) - entirely up to you

Nearly there on a public Snap7 repl.it instance :)

cymplecy commented 2 years ago

Just checked - I am using 4.2.5 :)

pixavier commented 2 years ago

After experiencing issues with test.mosquitto.org, I switched to https://www.emqx.com/en/mqtt/public-mqtt5-broker as a default broker. It seems a not very loaded public broker, and it works well. However, I cannot find a way to monitor its activity with MQTT Explorer, unlike test.mosquitto.org or mqtt.eclipseprojects.io. This last one seems to be very loaded, but I like it because it is a Mosquitto broker. If you use it and if it works well, let's set mqtt.eclipseprojects.io as the default broker.

cymplecy commented 2 years ago

I have converted all the blocks - not finished yet but should all work :)

This is a repl.it server instance link so you can have a play with it

https://snap7-mqtt.cymplecy.repl.co/snap.html#run:https://Snap7-MQTT.cymplecy.repl.co/Examples/mqtt4snap7ExtDev.xml&editMode

cymplecy commented 2 years ago

The extension and library files are available there too https://replit.com/@cymplecy/Snap7-MQTT#libraries/mqtt4snap.js https://replit.com/@cymplecy/Snap7-MQTT#libraries/mqtt4snap.xml

My current plan is to try and future proof the primitive extensions as much as possible, so that they can have extra parameters added to them, without it affecting any existing Snap7 projects

pixavier commented 2 years ago

I have converted all the blocks - not finished yet but should all work :)

This is a repl.it server instance link so you can have a play with it

https://snap7-mqtt.cymplecy.repl.co/snap.html#run:https://Snap7-MQTT.cymplecy.repl.co/Examples/mqtt4snap7ExtDev.xml&editMode

Wow ! I played a bit with it. It looks magnificent.

pixavier commented 2 years ago

The extension and library files are available there too https://replit.com/@cymplecy/Snap7-MQTT#libraries/mqtt4snap.js https://replit.com/@cymplecy/Snap7-MQTT#libraries/mqtt4snap.xml

My current plan is to try and future proof the primitive extensions as much as possible, so that they can have extra parameters added to them, without it affecting any existing Snap7 projects

I have a plan with the synchronous blocks. MQTT 5 has added the possibility of sending the idCallback in the packet header so that the implementation would be very clean. I haven't made up my mind because there are many MQTT 3.1 servers already running. What do you think ?

cymplecy commented 2 years ago

I've never used your request/response blocks as I don't have a use case for them but I did think that MQTT5 does seem to offer that sort of behaviour and facility.

All the public servers I know of support MQTT5 so I think you should go ahead and use it natively

If you would like, you could develop them using JS in 6.9 - once they are working - I can add add them into the extension primitives

cymplecy commented 2 years ago

Could you send me a link to an example project that uses your original request/response blocks because I don't really understand what they are for!

pixavier commented 2 years ago

Beyond the example at the end of Hello World!, here is a use case and a small presentation: https://www.hivemq.com/blog/mqtt5-essentials-part9-request-response-pattern

cymplecy commented 2 years ago

Aah- now I see what it is for :)

image

cymplecy commented 2 years ago

I am thinking that since this will hopefully become an "official" extension/library, can we simply call it "mqtt" and not "mqtt4snap"?

E.g the make the ./library files mqtts.js and mqtt.xml and use "mqt" instead of "m4s" as the 3 letter primitivie prefix?

As I am typing this, we might have to call the .js file something other than mqtt.js to avoid namespace collision - if your happy to go ahead with changing away from mqtt4snap then I'll try it out

pixavier commented 2 years ago

I am thinking that since this will hopefully become an "official" extension/library, can we simply call it "mqtt" and not "mqtt4snap"?

E.g the make the ./library files mqtts.js and mqtt.xml and use "mqt" instead of "m4s" as the 3 letter primitivie prefix?

As I am typing this, we might have to call the .js file something other than mqtt.js to avoid namespace collision - if your happy to go ahead with changing away from mqtt4snap then I'll try it out

MQTT4Snap! is Ok as an external extension, and I agree that 4Snap! redundancy is unnecessary if it is an official extension. No problem with going ahead.

cymplecy commented 2 years ago

I'm doing a lot of repeated clearing my cache and re-loading the library and having to re-create a working subscribe block and I'm wondering if we should change the order of the parameters

So instead of :

image

we could swap the order and make #1 the payload

image

I am thinking that the most common thing that someone would want to do would be to retrieve the payload

Obviously, if using wildcards, then the returned topic becomes equally as important but maybe not in the simplest cases?

It doesn't affect people like you and me who change the names to topic and payload but it may make things simpler / easier to comprehend for new people to MQTT

Your thoughts?

pixavier commented 2 years ago

I'm doing a lot of repeated clearing my cache and re-loading the library and having to re-create a working subscribe block and I'm wondering if we should change the order of the parameters

So instead of :

image

we could swap the order and make #1 the payload

image

I am thinking that the most common thing that someone would want to do would be to retrieve the payload

Obviously, if using wildcards, then the returned topic becomes equally as important but maybe not in the simplest cases?

It doesn't affect people like you and me who change the names to topic and payload but it may make things simpler / easier to comprehend for new people to MQTT

Your thoughts?

I had the same dilemma, and experience shows that it is better to put the payload first because it is the majority of cases. Now is the opportunity to change this.

cymplecy commented 2 years ago

👍 This is going very well :)

After changing the connect and publish extensions to use a JSON object of parameters

So the connection block currently looks like this

image

I did this so that future option additions do not affect any previous library blocks.

However, I have introduced a small bug that I'm currently trying to fix before updating the relp.it version

For information, I've now realised that I can develop using a standard JS block in the custom blocks while debugging and just have to copy / paste to the extension JS file when it is working properly

This now is speeding up development as I don't have to keep clearing my browser cache and reloading the blocks back in :)

cymplecy commented 2 years ago

On a coding note, I've noticed that I sometimes get an error in the console from the connection block and I think it is due to this line image

I have spent a long time trying to work out what is going on - my JavaScript skills are limited :)

I think that the connection status variable is not being in scope when a disconnection occurs.

If I comment out the line, I then get issues with the subscribe block as stage.mqtt[broker] has been deleted

Having tried a lot of things - I think the removing the whole .on close function is the cleanest solution.

I don't think it provides any real working benefit to the whole process.

What are your thoughts?

cymplecy commented 2 years ago

UPdated my repl.it site https://snap7-mqtt.cymplecy.repl.co/snap.html#run:https://Snap7-MQTT.cymplecy.repl.co/Examples/mqtt4snap7ExtDev2.xml&editMode

Library files now called mqttExtension.js and mqttExtension.xml

Primitives prefix is mqt (Jens says only use 3 letters)

I've left the JS code in the custom blocks to make it easy to edit, develop and debug but the code in mqttExtension.js should be the same as the JS code in the library blocks

pixavier commented 2 years ago

UPdated my repl.it site https://snap7-mqtt.cymplecy.repl.co/snap.html#run:https://Snap7-MQTT.cymplecy.repl.co/Examples/mqtt4snap7ExtDev2.xml&editMode

Library files now called mqttExtension.js and mqttExtension.xml

Primitives prefix is mqt (Jens says only use 3 letters)

I've left the JS code in the custom blocks to make it easy to edit, develop and debug but the code in mqttExtension.js should be the same as the JS code in the library blocks

Excellent !

cymplecy commented 2 years ago

Sorry it's been so long since last update

I just sent the extension off to Jens and Bernat

My current working development fork is at https://cymplecy.gitlab.io/Snap/snap.html.

Please try it out and see if it works for you - especially your response/request blocks as I haven't tested them at all.

Major changes since last version is to use variadic options as Jens has fixed the problem with them

So connect looks like this image

1st slot is username, 2nd is password, 3rd is keepalive time, 4th is callback

But the callback is treated specially - as long as it is the last parameter - it will be used - so it can be the only parameter as per the example.

Note: I've changed the value sent by the callback to "all" instead of the connack list that you used The reason for this is that users might want to use a broadcast in the callback. And due to the change in the default broadcast block, unless the user changes it to all sprites - it will fail unless "all" is supplied as the callback parameter (It is more complicated than this but I came up with this method as the simplest solution)

I hope this doesn't cause problems for any projects that you might use it in

Jens says him and Bernat are planning to include the mqtt.js library into Snap! itself so that Snap! can be used to access LAN brokers even if no internet is available.

If this works - then that should be good

Also, I have put back broker.emqx.io as default broker as it is more reliable than mqtt.eclipseprojects.io

I have also added code to add in the port and basepath (if needed) on the default brokers to make the options look simpler for new users of MQTT.

I have included information about this in the help comment for the connect block so that people know what to do for other brokers

    if (wsbroker == 'wss://broker.emqx.io') {
        wsbroker = wsbroker + ':8084/mqtt'
    } else if (wsbroker == 'ws://broker.emqx.io') {
        wsbroker = wsbroker + ':8083/mqtt'
    } else if (broker == 'mqtt.eclipseprojects.io') {
        wsbroker = wsbroker + '/mqtt'
    } else if (wsbroker == 'wss://test.mosquitto.org') {
        wsbroker = wsbroker + ':8081'
    } else if (wsbroker == 'ws://test.mosquitto.org') {
        wsbroker = wsbroker + ':8080'
    } else if (wsbroker == 'wss://simplesi.cloud') {
        wsbroker = wsbroker + ':8084'
    } else if (wsbroker == 'ws://simplesi.cloud') {
        wsbroker = wsbroker + ':8083'

Jens says it might be a few weeks before it gets added into Snap!

regards

Simon

pixavier commented 2 years ago

Hi Simon,

I've seen the fork code (not yet tried), and I've found it very nice and clean, including the "all" option in the disconnect block. Congrats.

I'm now upgrading the response/request blocks to MQTT 5, which has specific improvements for that. I hope to have it done in the next week.

Regards.

cymplecy commented 2 years ago

Jens has added it to the dev version :)

image

pixavier commented 2 years ago

Great !

cymplecy commented 2 years ago

Jens has pulled a PR I made to update the list of public brokers and I've added a new one I've found - broker.xmqtt.net

It is now in standard Snap! 7.2.0