Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MQTT protocol over bluetooth #15

Closed
wants to merge 24 commits into from
Closed

Conversation

Tasm-Devil
Copy link

With this commit, the MQTT class now also supports Bluetooth. I wrote a new socket class based on Adafruit_CircuitPython_ESP32SPI. There is also an example. I split the MQTT class somewhat so that there is a clear separation between MQTT and the underlying protocol.

I hope this work will be useful to others.

Warm greetings from Germany.

@@ -46,7 +46,7 @@
from micropython import const
import adafruit_logging as logging

__version__ = "0.0.0-auto.0"
__version__ = "0.1.0-auto.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this as-is. The build scripts replace it with the git tag.

@Tasm-Devil
Copy link
Author

OK. It's ready to push.

@brentru
Copy link
Member

brentru commented Jan 6, 2020

Hi @Tasm-Devil - Travis is failing here: https://travis-ci.com/adafruit/Adafruit_CircuitPython_MiniMQTT/builds/142054500#L299

Once this check passes, I'll review the PR.

@Tasm-Devil
Copy link
Author

Can you point to the code? I can't see the bug.

@brentru
Copy link
Member

brentru commented Jan 22, 2020

@Tasm-Devil There are a few PyLint errors with the code. These aren't bugs, they are syntax issues.

For example, on Line 301 of the attached URL: https://travis-ci.com/adafruit/Adafruit_CircuitPython_MiniMQTT/builds/142054500#L2301

************* Module adafruit_ble_socket
C:124, 0: Line too long (102/100) (line-too-long)

On line 124 of the adafruit_ble_socket module (here: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/pull/15/files#diff-2e0ccaa8f54e85ee63d7eadf896bd7cfR124), the line is 2 characters over the allowed limit (100 chars.). You could resolve this error by moving to the next line and indenting, or reducing the amount of characters on that line.

Please work through each error until they're resolved. Then, re-commit your code. We have a guide/reference for this on the Adafruit Learning System: https://learn.adafruit.com/improve-your-code-with-pylint/pylint-errors#working-through-pylint-errors-5-5

@Tasm-Devil
Copy link
Author

fixed

.pylintrc Outdated Show resolved Hide resolved
adafruit_minimqtt.py Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change .pylintrc.

Instead, please fix the errors thrown by pylint or add a line to disable pylint checking (# pylint: disable=some-message,another-one) above the line which you feel is erroneous. I'll review the lines and provide feedback.

Copy link
Author

@Tasm-Devil Tasm-Devil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change of the pylintrc was not intened. Sorry.

adafruit_minimqtt.py Outdated Show resolved Hide resolved
@brentru
Copy link
Member

brentru commented Feb 3, 2020

@Tasm-Devil what hardware and code did you use to test this class?

@Tasm-Devil
Copy link
Author

I tested it on the Feather nrf52840. The Software was my BLE2WebService site, you can find here: https://github.com/Tasm-Devil/BLE2WebService

@brentru
Copy link
Member

brentru commented Feb 4, 2020

Ok, will test with a CLUE board.

@Tasm-Devil
Copy link
Author

Nice. I'd like to get my hands on one 🙂

@brentru
Copy link
Member

brentru commented Feb 4, 2020

Ok, where is examples/adafruit_ble_socket.py originally sourced from? You have ladyada as the author, but it's not an official adafruit repository.

If you made this module, please submit a pull request for it to the Adafruit_CircuitPython_BLE library (https://github.com/adafruit/Adafruit_CircuitPython_BLE). I feel it should be located there, ESP32SPI isn't part of MiniMQTT.

@tannewt
Copy link
Member

tannewt commented Feb 4, 2020

Why do you need MQTTOverWifi and MQTTOverBluetooth classes? Can you encapsulate the wifi and bluetooth details so that the MQTT stuff doesn't need to know about it?

@Tasm-Devil
Copy link
Author

@Bentru The socket is borrowed from here: adafruit_esp32spi/adafruit_esp32spi_socket.py
I agree, that this socket-file should be located somewhere else and not in the examples folder. But I thought, temporarily it's ok.

@Tasm-Devil
Copy link
Author

Why do you need MQTTOverWifi and MQTTOverBluetooth classes? Can you encapsulate the wifi and bluetooth details so that the MQTT stuff doesn't need to know about it?

I need those two subclasses because they use different interfaces. I think, that the parent-class should only do the MQTT-stuff and the children-classes worry about the two interfaces and how to establish a network connection. You can see the differences in the constructors of these subclasses.

How should we proceed now?

@tannewt
Copy link
Member

tannewt commented Feb 12, 2020

@brentru I think a new BLE_Socket library would be best. We'll want to move all of the Wifi stuff out of here as well and into the ESP32SPI socket. MQTT shouldn't need to worry about the transport details at all.

@Tasm-Devil
Copy link
Author

@brentru I think a new BLE_Socket library would be best. We'll want to move all of the Wifi stuff out of here as well and into the ESP32SPI socket. MQTT shouldn't need to worry about the transport details at all.

Sounds very good to me. This is how it should be done.

@kattni
Copy link
Contributor

kattni commented Mar 30, 2020

@Tasm-Devil Are you still working on this? We have made some changes to the library that will now require you to update this PR to resolve merge conflicts. Please let us know if you need assistance with that.

@Tasm-Devil
Copy link
Author

Yes, I will update everything. But I will create a new Pull request when I am ready.
Is a new repo for the BLE_Socket library already created by adafruit or should I create a new repo and you fork it?

@Tasm-Devil
Copy link
Author

@brentru I'm looking at your revision of the MQTT class. I think you unfortunately made the same assumption regarding the interface that my interface cannot fulfill and why I had to create two subclasses.

You think of the interface class of adafruit_wiznet5k.py or adafruit_esp32spi.py
These two classes have the method unpretty_ip (), which my interface class nordic.py does not have and does not need either.

This means that because of lines 123 and 240 my plan to use this class via WebSocket over Bluetooth fails.

@Tasm-Devil
Copy link
Author

I think it's ready to merge now. Althought the 64B Buffer size of the Bluetooth Service class is quite small for more than one subscripted message, it works like a charm. Thanks to bentru for cleaning up that class.

@jposada202020
Copy link
Contributor

@brentru and @tannewt could you take a look a this, I could work out with @Tasm-Devil the merge conflict in case this is good for you folks. Thanks

@tannewt
Copy link
Member

tannewt commented May 14, 2021

I'd like to see it use the new socketpool/socket model. I think this library has been switched over. It feels like this could be a separate library too. What needs to be on the other end of the BLE connection? It seems weird to me that it uses a UART service.

@brentru
Copy link
Member

brentru commented May 14, 2021

I agree with Scott w.r.t updating + making it a sep library if anyone wants to take it on...but,

The more I think about it, I am confused about this PR... The MQTT 3.1.1 protocol (which this library follows) transports over TCP/IP, but does not specify BLE as a transport [MQTT Specification, 4:2 Network Connections]:

The transport protocol used to carry MQTT 3.1 was TCP/IP as defined in [RFC793]. TCP/IP can
1207 be used for MQTT 3.1.1. The following are also suitable:

- 1208 TLS [RFC5246]
- 1209 WebSocket [R]

A different MQTT specification, MQTT-SN, defines a "virtual connection" which could be used with BLE. That would be another CircuitPython library, though.

@jposada202020
Copy link
Contributor

@Tasm-Devil Let me know what yo think. But as we see it this would need to be in another library. let me know thanks

@evaherrada evaherrada changed the base branch from master to main June 7, 2021 18:34
@Tasm-Devil
Copy link
Author

Tasm-Devil commented Jul 4, 2021

The example is up to date now. The many changes done in the last year are so good, that my little example works very good with this refactored MQTT Library. I wouldn't put the adafruit_ble_socket.py in another library because it's only a little helper socket class which I consider as a quick fix for this special example.

But the adafruit_ble_socket.py has to be in the lib folder currently.

@Tasm-Devil
Copy link
Author

I'd like to see it use the new socketpool/socket model. I think this library has been switched over. It feels like this could be a separate library too. What needs to be on the other end of the BLE connection? It seems weird to me that it uses a UART service.

This is on the other side of the Bluetooth Connection. It will open an Secure Websocket to the broker. I want to use it in my class to inspect MQTT-Messages on the fly. The Page needs a little work but its cool to see the MQTT-Packages and try to understand them by reading the RFC.

@FoamyGuy
Copy link
Contributor

Closing this for now. My understanding reading this chain is that we would want this functionality introduced in a separate library.

@Tasm-Devil if you're interested in working on it still you could move the bluetooth implementation into it's own repo and we could add it to the community bundle if you like. If you do want to do that and need help you can ping me here or join the discord.

@FoamyGuy FoamyGuy closed this Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants