peterhinch / micropython-mqtt

A 'resilient' asynchronous MQTT driver. Recovers from WiFi and broker outages.
MIT License
549 stars 116 forks source link

Working with a wired LAN #56

Closed atitoff closed 1 year ago

atitoff commented 3 years ago

Will the library work with a wired network, such as a ESP32 + ethernet LAN8720A?

kevinkk525 commented 3 years ago

Since this library controls the WLAN component directly, it can't be used with a wired network (as it is). But it shouldn't be difficult to adapt it for a wired network. The easiest way might be to use my fork which has unix port support, which means I already made changes to have it working without any network interface (because the unix port doesn't have a network interface). This might already be enough to work with a wired network if you start/connect that network manually before starting the library and if that network is stable and doesn't need any manual reconnects of the network interface. So for testing you'd just need to replace the platform strings to "simulate" having a linux platform so no WLAN is being used:

ESP8266 = platform == 'esp8266'
ESP32 = False
PYBOARD = platform == 'pyboard'
LINUX = True 

(My fork: https://github.com/kevinkk525/micropython-mqtt Note that the constructor works slightly different, check the README. Apart from that, it's pretty much a drop-in replacement)

atitoff commented 3 years ago

Many thanks! Your fork is working! So far only tested on Linux

if platform == "linux":
    config["client_id"]="linux"
kevinkk525 commented 3 years ago

Note that I have never worked with a wired LAN and I don't know if manual (re-)connects might be needed if the network adapter drops/loses the connection. In that case you'd need to program a workaround to reconnect the LAN and make a change to the constructor that lets you define the network interface so you can choose between WLAN and LAN.

atitoff commented 3 years ago

Debugged for Linux. Everything is working. Transferred to the board ESP32. The wired lan and webrepl works. The MQTT does not start.

def mqtt_callback(topic, msg, retained):
    # if topic.startswith(b'dali/'):
    #     dali.receive_from_mqtt(topic, msg)
    print(topic, msg, retained)

async def mqtt_conn_handler(client):
    await client.subscribe('dali/#', 1)

mqtt = MQTT(host='192.168.1.107', user='alex', password='bh0020', subs_cb=mqtt_callback, conn_handler=mqtt_conn_handler,
            debug=True)

async def main():
    print('mqtt.connect')
    await mqtt.connect()
    print('mqtt.connected')
    # await dali.run(mqtt.client)

    await asyncio.sleep(1)
MPY: soft reboot
WebREPL daemon started on ws://0.0.0.0:8266
Started webrepl in normal mode
mqtt.connect
kevinkk525 commented 3 years ago

Is the webrepl reachable? Because it doesn't show the real ip Adress in what you posted. Can you connect to your broker manually using socket.socket()?

atitoff commented 3 years ago

image

image

Later I will watch the Wireshark tcp exchange ...

kevinkk525 commented 3 years ago

Webrepl runs as a server so maybe it behaves differently than outgoing connections. Does ntptime sync work or general network connections? Eg dns lookups Did you disable both wlan interfaces?

atitoff commented 3 years ago

ntptime works

print(ntptime.time())
await asyncio.sleep(5)

671432518
671432523
671432528
671432533

this works too

async def echo(reader, writer):
    data = await reader.read(100)
    print(data)
    writer.write(data)
    await writer.drain()
    writer.close()

async def main():
    asyncio.create_task(asyncio.start_server(echo, "192.168.1.34", 8034))
kevinkk525 commented 3 years ago

Looks line ntptime uses udp sockets. Can you test normal sockets? Just connect them to the broker like the examples of an echo client for python.

It's not surorising to me that servers work, they care less about where a connection comes from, while an outgoing connection might try to use a different interface. Did you explicitly disable wlan interfaces?

atitoff commented 3 years ago

Ok, I'll try. I did not turn on the wireless interface.

boot.py

import esp

esp.osdebug(None)
import webrepl
import machine
import network

lan = network.LAN(mdc=machine.Pin(23), mdio=machine.Pin(18), power=machine.Pin(16), phy_type=network.PHY_LAN8720,
                  phy_addr=1, clock_mode=network.ETH_CLOCK_GPIO0_IN)
lan.active(True)
lan.ifconfig(('192.168.1.34', '255.255.255.0', '192.168.1.1', '192.168.1.1'))
webrepl.start()
kevinkk525 commented 3 years ago

Not turning it on doesn't necessarily mean it is off. Often it is needed to explicitly disable the wireless ap. So maybe try with st.active(False) and ap.active(False)

atitoff commented 3 years ago
import esp
esp.osdebug(None)
import webrepl
import machine
import network

wlan = network.WLAN(network.STA_IF)
ap = network.WLAN(network.AP_IF)
wlan.active(False)
ap.active(False)

lan = network.LAN(mdc=machine.Pin(23), mdio=machine.Pin(18), power=machine.Pin(16), phy_type=network.PHY_LAN8720,
                  phy_addr=1, clock_mode=network.ETH_CLOCK_GPIO0_IN)
lan.active(True)
lan.ifconfig(('192.168.1.34', '255.255.255.0', '192.168.1.1', '192.168.1.1'))
# webrepl.start()

Implemented asyncio TCP echo server

async def echo(reader, writer):
    data = await reader.read(100)
    print(data)
    writer.write(data)
    await writer.drain()
    writer.close()

async def main():
    asyncio.create_task(asyncio.start_server(echo, "192.168.1.34", 8034))

Only the first 8 sessions work. Then the port is closed. Sessions are normal. Here is one session for an example

image

I'll try later without async

kevinkk525 commented 3 years ago

As mentioned in my earlier replies, please test the echo CLIENT or just a simple, manual outgoing tcp connection to a server (either python echo server on your PC or to your mqtt broker directly) to see if outgoing connections work correctly. We already established that incoming connections work.

atitoff commented 3 years ago

TCP client works without problems

async def tcp_echo_client():
    reader, writer = await asyncio.open_connection('192.168.1.107', 8888)
    writer.write(b'test')
    await writer.drain()
    data = await reader.read(100)
    print(data)
    writer.close()
    await writer.wait_closed()

async def main():
    await asyncio.sleep(1)
    i = 0
    while True:
        await asyncio.sleep(2)
        i += 1
        await tcp_echo_client()
        print(i)
MPY: soft reboot
b'test0'
1
b'test1'
2
b'test2'
3
b'test3'
4
b'test4'
5
b'test5'
6
b'test6'
7
b'test7'
8
b'test8'
9
b'test9'
10
b'test10'
11
b'test11'
12
b'test12'
13
b'test13'
14
b'test14'
15
kevinkk525 commented 3 years ago

That's good. Can you please try one without asyncio too? And opening a socket manually to that server without specifying any arguments, like the mqtt_as library does? Like socket.socket() Maybe there's some problem with that. And/or maybe try mqtt again now that wlan is definitely disabled.

atitoff commented 3 years ago

if the platform is forcibly specified, it starts working, but with errors

image

kevinkk525 commented 3 years ago

Please add 119 and 118 to the list in line 38 (so that it looks like line 36). Forcing the platform to Linux is required otherwise mqtt_as will use the wifi on the esp.

atitoff commented 3 years ago

image

kevinkk525 commented 3 years ago

Hmm that means it couldn't connect.. Can you try to connect manually to the broker and your echo server? (without asyncio)

atitoff commented 3 years ago

TCP socket works

image

kevinkk525 commented 3 years ago

Great, then try to connect to your mqtt broker the same way and send a message.

atitoff commented 3 years ago

image

image

kevinkk525 commented 3 years ago

Sorry I'm a bit out of ideas now. The previous test with the echo client and server worked so I would expect this to work now too (unless there is some error in your mqtt setup, credentials or similar. You don't have another mqtt client connected with the same ID?). But at least there's progress, it gets a connection even if not successful. Could you post some more output of mqtt with debug on? I'd like to see your whole code and detailed output. (preferably as text instead of pictures)

atitoff commented 3 years ago

Kevin, thank you very much for your great desire to help! I wrote a socket based gateway Dali/TCP. Everything works fine. I'll post it later in my repository. For example:

Code ```python class Dali: def __init__(self, in_port, out_port, tcp_port=8998): self._in_port = in_port self._tcp_port = tcp_port self._rmt = esp32.RMT(0, pin=Pin(out_port), clock_div=128) self._command_queue_len = 0 self._busy = asyncio.Lock() self._dali_send_buffer = [1, 0] + [0]*32 async def _serve(self, reader, writer): print('command_queue_len:', self._command_queue_len) if self._command_queue_len > 10: writer.write(b'buffer_full') await writer.drain() writer.close() await writer.wait_closed() return self._command_queue_len += 1 command = await reader.read(8) async with self._busy: try: resp = await asyncio.wait_for(self.command_handler(command), timeout=1.0) except asyncio.TimeoutError: resp = b'timeout' writer.write(resp) await writer.drain() writer.close() await writer.wait_closed() self._command_queue_len -= 1 async def start_tcp_server(self): asyncio.create_task(asyncio.start_server(self._serve, "0.0.0.0", self._tcp_port)) async def send(self, b): self._rmt.write_pulses(self._byte_to_rmt(b), start=1) async def command_handler(self, cmd): print(cmd) ret = b'unknown' if len(cmd) != 3: return ret command = cmd[2] address = cmd[1] value = cmd[0] print('command', command, 'address', address, 'value', value) if command == 1 and 0 <= address < 64: ret = await self._cmd_set_level(address, value) elif command == 2 and 0 <= address < 15: ret = await self._cmd_set_level_grp(address, value) return ret async def _cmd_set_level(self, address, value): print('cmd_set_level address:', address) return b'OK' async def _cmd_set_level_grp(self, address, value): print('cmd_set_level_grp address:', address) if value == 255: value = 254 # send address = address | 0b10000000 await self.send(bytearray([value, address])) await asyncio.sleep_ms(20) return b'OK' async def _cmd_find(self): pass def _byte_to_rmt(self, b): b = bytes(b) for i in range(16): if b[i // 8] & 1 << i % 8 != 0: self._dali_send_buffer[i * 2 + 2] = 1 self._dali_send_buffer[i * 2 + 3] = 0 else: self._dali_send_buffer[i * 2 + 2] = 0 self._dali_send_buffer[i * 2 + 3] = 1 rmt = [] compared = False for i in range(34): try: if compared: compared = False continue if self._dali_send_buffer[i] == self._dali_send_buffer[i + 1]: rmt.append(520) compared = True else: rmt.append(260) except IndexError: if i % 2: rmt.append(1300) # 260*5 else: rmt.append(260) rmt.append(1040) # 260 * 4 pass return rmt ```
spacemanspiff2007 commented 3 years ago

I'm too looking for an async mqtt client that works with a wired connection and I've found various async mqtt clients that implement more or less the same functionality.

Imho it would make sense to move the resilient interface handling into a own class. That way the async client could be interface agnostic. An additional benefit would be that it's easy to modify the driver for different hardware platforms.

Have you guys thought about that and was it a conscious decision against it or have the mqtt drivers "grown" to the state they are now?

peterhinch commented 3 years ago

Coping with the inherent unreliability of WiFi was a core concept with the aim being to provide reliable MQTT on ESP8266 and ESP32. It was subsequently successfully tested on Pyboard D.

A version of mqtt_as for a wired LAN would be a worthwhile project, which could result in a substantial simplification. A first step would be to consider whether to adapt mqtt_as for wired connectivity or to port an existing asynchronous MQTT library to MicroPython.

I am busy with another project so this isn't something I plan to tackle at present.

spacemanspiff2007 commented 3 years ago

Thanks for your reply. I think I might have left some things unclear. I have a board that provides and additional ethernet chip and I currently I am not sure if I can use it. So I'd like to have the possibility to either use Wifi or Ethernet, depending on the credentials I enter. For this to work it must be possible to change the interface for the mqtt_as.

Coping with the inherent unreliability of WiFi was a core concept with the aim being to provide reliable MQTT on ESP8266 and ESP32.

For the change to work, the interface must be abstracted so mqtt_as is agnostic the interface it is using. As for my question: If I read between the lines the library just "grew" the way it is and different interfaces were out of scope.

I am currently abstracting my interface handling in a class as an agnostic interface. Are you interested in a PR or should I just use it locally?

kevinkk525 commented 3 years ago

I never worked with wired lans on microcontrollers but even though the are more stable than wifi, there are still things to consider like: cable reconnects, interferences from nearby devices that might scramble communication/network link temporarily and maybe other reasons why a message could fail or even the interface connection would fail temporarily. How is that case handled by micropython? Maybe a manual/automatic reconnect of the interface will be needed. But in either case, the mqtt client needs to deal with a transmission loss. So in my opinion the mqtt approach for both interfaces wouldn't be different at all. All that would be required is to separate the interface specific code from the mqtt client code which (by only quickly thinking it through) comes down to the code during every connect (because also the wifi is being reconnected) and the code to check if the network link is up. Those can easily be separated into a new class/submodule that inherits from the mqtt base class. The interface class would need to provide a coroutine that is called to connect the interface before connecting to a broker, a coroutine that is called for checking the interface link and a coroutine that is called to disconnect from the interface.

(To support the unix port on my fork I made some changes that ignore all interface code in mqtt_as and the changes are rather small, so it shouldn't be too hard to separate the interface specific code from the mqtt client)

peterhinch commented 3 years ago

If I read between the lines the library just "grew" the way it is and different interfaces were out of scope.

The project had two specific objectives:

Wired interfaces were never considered.

Are you interested in a PR or should I just use it locally?

My own view is that it depends :) I would be reluctant to significantly increase code size, as this would risk breaking ESP8266 applications. I would also have a problem testing any changes as I would need to buy wired microcontroller hardware.

If the changes could be implemented in a subclass, leaving existing code unchanged, that would be ideal but I doubt this is practicable.

Another option would be to implement (and document) the changes in a fork. That would enable you to merge any bugfixes we might implement. I would be happy to update our docs to direct users who need your functionality to the fork.

@kevinkk525 I would be interested to hear your views.

spacemanspiff2007 commented 3 years ago

I would be reluctant to significantly increase code size, as this would risk breaking ESP8266 applications.

Imho device specific implementations could be split out in different files. E.g. a Wifi handler for ESP, pyboard, etc. While total code size is bigger, it's possible to only deploy the wifi handler for the device resulting in a smaller code size overall.

The interface class would need to provide a coroutine that is called to connect the interface before connecting to a broker, a coroutine that is called for checking the interface link and a coroutine that is called to disconnect from the interface.

Since I additionally run a small webserver, I need to recreate it, too. Otherwise it won't serve the pages properly. I've played a little bit around and come up with a subscribe mechanism. That way the class instance can be passed into the library and it can register the callbacks accordingly.

# Wifi specific implementation
class IfWifi(InterfaceBase):
    def __init__(self):
        super(IfWifi, self).__init__(network.WLAN(network.STA_IF), auto_reconnect=True)

    async def do_connect(self):
        # interface specific connect logic
        self._if.connect(CONFIG_WIFI['ssid'], CONFIG_WIFI['pass'])

        while self._if.status() == network.STAT_CONNECTING:
            await sleep_ms(200)
        ...

    async def do_disconnect(self):
        # disconnect logic
        pass

WIFI = IfWifi()

async def disconnected():
    print('disconnected)

async def connected():
    print('connected)

# subscribe callbacks through base class
WIFI.on_connect(connected)
WIFI.on_disconnect(disconnected)

# interface that can be used by the code
await WIFI.connect()
await WIFI.disconnect()
await WIFI.check()

# vars that make additional logic easier
WIFI.is_connected
WIFI.has_connected
WIFI.first_connect
WIFI.is_connecting
kevinkk525 commented 3 years ago
* Resilient behaviour on wireless links, initially on ESP8266. Specifically fixing numerous issues with the official library.

Wired interfaces were never considered.

I think it may be time for the library to "evolve". There are more and more microcontrollers with micropython and they have different ways of connecting to a network (wifi, ethernet, maybe even some sort of bridges/proxy for boards without wifi, unix/windows ports).

Another option would be to implement (and document) the changes in a fork. That would enable you to merge any bugfixes we might implement. I would be happy to update our docs to direct users who need your functionality to the fork.

This library is the most mature mqtt client based on uasyncio and considering the small amount of developers, I think it would be best to concentrate the efforts into maintaining a single library that fits all use-cases. I already maintain a fork with several changes to increase compatibility with other ports (unix) and extend the functionality (unsubscribe, ...). Having multiple forks is imho not the way to go. It's not good if users have to read up on the differences between multiple forks and the approach is prone to frustration because developers might naturally stop supporting their fork if they move on (or due to other reasons), also any important changes in the main repository will have to be pulled by all forks. So over time forks will grow apart or become unmaintained.

* Resilient behaviour on wireless links, initially on ESP8266. Specifically fixing numerous issues with the official library.

A resilient behaviour will be needed for every kind of connection because sometimes things go wrong at completely unexpected points. Admittedly, the wifi link is the most unreliable but afaik the esp8266 is the board with the lowest amount of RAM and boards without wifi often have lots of RAM, so even keeping a bit of "unneccessary" overhead in terms of unneeded resiliency will not make a difference on those boards.

My own view is that it depends :) I would be reluctant to significantly increase code size, as this would risk breaking ESP8266 applications. I would also have a problem testing any changes as I would need to buy wired microcontroller hardware.

Keeping the code size down on esp8266 is of course important and I think this could work. (A thought beyond the scope of this issue might be to think about offering a prebuilt firmware with certain popular and well maintained libraries like mqtt_as so that users don't have to either build their own firmware or cross-compile it or worse, upload the .py files and waste half the RAM on the esp8266. A small change in codesize would be irrelevant if the library was frozen but new users won't start freezing modules and therefore run into trouble while comiling their projects on the board.)

As for testing: I understand that you will have to rely on other users to test the implementation and I think that shouldn't be a problem. If the mqtt code is separated from the hardware, you can just add hardware modules that have been tested and list the tested modules in the README. Others users can then easily add more modules or create PRs to have them added.

If the changes could be implemented in a subclass, leaving existing code unchanged, that would be ideal but I doubt this is practicable.

The easiest way might be to provide a base network class showing the structure:

class BaseInterface:
    async def connect(first_connect: bool):
        # connect the interface 
        return True

    async def disconnect():
        # disconnect the interface
        return True

    async def isconnected():
        # return the status of the connection
        return True

Then depending on the hardware you can just subclass this and make it connect to a wifi or other interface. The existing wifi interface code in mqtt_as can be easily moved into such a class.

The mqtt_as class would then just take such an interface class in the constructor (or create an interface class if the connection type is given, e.g. "wlan"). The rest of the code would then just call the corresponding coroutine of the interface class.

This way the code size shouldn't change significantly and we can implement a compatibility mode for existing installations by creating a wifi interface automatically if the config dict contains the wlan configurations. At the same time this change makes it possible to support any kind of hardware and ports. The unix port would just automatically create an empty Interface class (or the code ignores the interface if it is set to None). Any ethernet or other interface can be created by the user itself and/or tested modules can be added to the repository as separate files. Then we have support for multiple ports and interfaces while never needing any change to the basic mqtt library and without increasing the code size for any other port/interface when adding a new interface.

(when doing all these changes we could also think about making the socket providable in the constructor in case someone wants to use some kind of proxy like a wlan link over SPI or similar)

spacemanspiff2007 commented 3 years ago

@kevinkk525 Thank you for your detailed reply. I think we both are thinking in the same direction. However I think having the possibility to subscribe to interface changes is a very important part. If I have two libraries and one disables the interface the other should have the possibility to be notified.

Another benefit would be that the connection logic for wifi is only implemented once and can be shared across projects. We could create a WifiEsp32 and a WifiPyBoard class and the user can just instantiate the corresponding class and benefit from robust wifi handling.

I've created a small ghist of my current implementation and I'd really like to hear your ideas and feedback about it.

kevinkk525 commented 3 years ago

However I think having the possibility to subscribe to interface changes is a very important part.

Sadly micropython doesn't offer a way to subscribe to interface changes so all we can do is poll the interface status and then call a callback. (but I think I understand what you mean)

At the moment mqtt_as does have a callback for wifi state changes and for compatibility reasons this should be retained. I didn't think about that part in my last message and I agree that this should be handled by the interface class too. But it's easy to implement, you just need to call the callback in the coroutine interface.isconnected() whenever the state changes.

Another benefit would be that the connection logic for wifi is only implemented once and can be shared across projects.

It's entirely up to you to create an interface class that can be used across all your projects. This new approach offers the user a lot of options and freedome.

I've created a small ghist of my current implementation and I'd really like to hear your ideas and feedback about it.

This approach is rather complex. It might be good for your project but generall it's like taking a sledgehammer to crack a nut. For most applications the current simple approach would be enough so I wouldn't go overboard with complex interface classes. The standard should be simple. Everyone can still build upon those or replace them with a complex class like yours. Having multiple interfaces or SSIDs or a mix of sta and ap would all be possible with a custom interface implementation.

The only thing to think about is that mqtt_as currently requests and interface reconnect every time the connection breaks. This might not be a desired behaviour on interfaces other than wifi or if you have multiple projects using the same interface. To solve this problem we could have a reconnect coroutine called by mqtt_as. Then the interface implementation can decide if a reconnect is really required or if mqtt_as should just try to connect the socket again without bothering the interface (could even implement to reconnect the interface if a socket connection fails 3 times within 20 seconds or something like that. Might be interesting in wifi environment, ethernet will likely not need it).

spacemanspiff2007 commented 3 years ago

It's entirely up to you to create an interface class that can be used across all your projects. This new approach offers the user a lot of options and freedome.

What I meant is that currently there are device specific workarounds for wifi connection (e.g. pyboard, esp32, esp8266). If there is a reference implementation for the device provided by this library one can work from there and benefit from robust connection handling.

But it's easy to implement, you just need to call the callback in the coroutine interface.isconnected() whenever the state changes.

That's what I meant. Imho it's more elegant to just subscribe to the changes than to poll the state from every library and then to re implement the on-change-logic in every implementation. Because in the implementation I typically want to do something when the:

The only thing to think about is that mqtt_as currently requests and interface reconnect every time the connection breaks.

That's why I differentiated between the connect request by the code (connect) and the actual connect implementation (do_connect). Overriding connect can be used to filter connect requests issued by libraries without having to touch the connect implementation.

This approach is rather complex. It might be good for your project but generall it's like taking a sledgehammer to crack a nut. For most applications the current simple approach would be enough so I wouldn't go overboard with complex interface classes.

Hm - imho it's not too bad. If you look at the usage example it's rather simple. There is some complexity through the possibility of the callbacks, but without those reusing is really hard and I think the resulting code simplifications through the subscribe mechanism are worth it.

Do you think we can work together on a implementation so we/I can publish a stable package?

kevinkk525 commented 3 years ago

What I meant is that currently there are device specific workarounds for wifi connection (e.g. pyboard, esp32, esp8266). If there is a reference implementation for the device provided by this library one can work from there and benefit from robust connection handling.

Yes that would be my goal too. Have a good reference implementation for the wlan interface for those boards within the repository (the code for that already exists within mqtt_as anyway) and then anyone who needs different interfaces can just create those themselves starting from the existing "blueprint". Then you can either use a minimalistic interface approach or a more complex one like yours, depending on the project needs.

Imho it's more elegant to just subscribe to the changes than to poll the state from every library and then to re implement the on-change-logic in every implementation.

Yes, just one class is needed that polls the interface and since we're programming with uasyncio, it could theoretically be another task that continuously polls the interface and serves all event subscribers. That's of course not how mqtt_as currently works but we can think about implementations.

That's why I differentiated between the connect request by the code (connect) and the actual connect implementation (do_connect). Overriding connect can be used to filter connect requests issued by libraries without having to touch the connect implementation.

I see, yes I do things like that too in other applications but typically you would have a public interface "connect" and an internal, private interface "_connect".

Hm - imho it's not too bad. If you look at the usage example it's rather simple.

Simplicity in usage is not what I meant. Within the mqtt_as project we need a simple approach in terms of code size and execution requirements. The goal is to separate the core mqtt client from the hardware interface without adding complexity and further features. The hardware interface can always be extended in separate repositories.

Do you think we can work together on a implementation so we/I can publish a stable package?

I think we can do that. Let's each write something up about the goals and how to approach and solve the problems. Then we can discuss the differences better. Maybe open a new issue for this. I'll also write something up (hopefully) but it might take me a few weeks.

kevinkk525 commented 2 years ago

Guess I was faster than I thought, took an hour today, see https://github.com/peterhinch/micropython-mqtt/pull/57

spacemanspiff2007 commented 2 years ago

Why did you bind the socket to the Interface? It won't work if I have two applications that are using the socket.

I still think it makes sense to add the possibility to subscribe to the respective connect and disconnect. Even with only the mqtt_as as supported library it's more elegant to put it the logic in the interface handler and it doesn't make a difference in code size. It however simplifies code logic on the library side. Also why do Interface implementations have to override "private" methods?

Additionally I thought this would be more of a discussion so we can iterate to a good solution.

kevinkk525 commented 2 years ago

Why did you bind the socket to the Interface?

Only the socket module is bound to the interface so a custom socket implementation could be used. The socket object itself is still bound to the mqtt object.

It won't work if I have two applications that are using the socket.

I don't understand what you mean by that. Could you please explain that more detailed?

I still think it makes sense to add the possibility to subscribe to the respective connect and disconnect.

Currently the callback is always called after a disconnect or a connect. However, it might indeed be better to move the callback code into the (dis)connect methods rather than the isconnected method because this one would have to be actively called for the callbacks to work. Edit: Pushed a commit to also execute the callbacks if a manual (dis)connect was successful so it doesn't rely on the polling of isconnected(). Still executing callbacks if state changes in isconnected() as the connection could drop without a manual disconnect.

Even with only the mqtt_as as supported library it's more elegant to put it the logic in the interface handler and it doesn't make a difference in code size. It however simplifies code logic on the library side.

Not sure what you mean by that.

Also why do Interface implementations have to override "private" methods?

I think that's how it is done. A private method is by definition private and not a public method, therefore not part of the public interface. The "do_connect" method in your code is a public method but it is not supposed to be called by any external code. It is only supposed to be called by the interface class itself, therefore it should be a private method. A class inheriting from the base class would then override that private method.

Additionally I thought this would be more of a discussion so we can iterate to a good solution.

It still is. I just provided an untested draft and as written in the PR I am looking for feedback. Please present your concerns, different opinions, solutions, etc. I opened the PR to provide a space to discuss the changes.

spacemanspiff2007 commented 2 years ago

@kevinkk525 : Sorry if my comment sounded rude or annoyed- I've had a rough couple of days.

I don't understand what you mean by that. Could you please explain that more detailed?

I missed that it's only the socket module.

I'll leave my comments in your PR.

kevinkk525 commented 2 years ago

It's all good. Thanks for your comments. Hope your days get better.

schaefer01 commented 1 year ago

just wanted to know if any progress has been made with wired ethernet, and I left a message with the person who was working on it last

peterhinch commented 1 year ago

No. Unfortunately @kevinkk525 is unable to work on this, and I think something more radical is required.

This library was originally developed to fix the problems with the official library on ESP8266. Coverage was extended to other WiFi capable platforms as they emerged. By contrast MQTT was developed to provide reliable communications over arbitrary unreliable media including radio links where there is no underlying network architecture.

Rather than extending mqtt_as to handle additional network types I would like to see it rewritten so that it can support non socket based communication. A socket module could be dropped in where required. A module to support WiFi disconnect and reconnect could also be linked if necessary. This is beyond anything I could undertake alone. I have had some discussion with @jimmo on this topic - the maintainers do seem interested in an official MQTT library that actually works. It's unclear where the effort will come from to develop it.

beetlegigg commented 1 year ago

Almost all of my projects on microcontrollers use your mqtt_as program for comms over wifi to my rpi's or my mac computer, but I would also like a wired ethernet asyncio capability as well. I'm not advanced in my python to really follow your program without a deal of head scratching so I cant offer to assist in the programming much as I would like to, but I do volunteer to do end user testing for any project that may come to fruition. I do hope you get the necessary assistance for your proposed MQTT library as it would help to future proof it and perhaps make it part of the micropython distribution. If you got a bit carried away could it lead to an async mqtt library for python 3 as well ..... sorry thats a bit cheeky.

peterhinch commented 1 year ago

Closing as I can't currently see a way forward on this.