maintain time.monotonic precision by using adafruit_ticks #210
Conversation
I originally made the change using the adafruit_ticks library, and it seemed to work just fine. But I believe Justin suggested not including an additional dependency since they would want this library to work with the esp32spi, which is frozen: |
|
I would not worry a lot about the new dependency: we can freeze adafruit_ticks if needed. There are only a few boards that don't have monotonic_ns: basically boards that don't have longints: SAMD21 boards without external flash, and a few STM boards with low flash. I don't think these boards are used for MQTT with such low RAM anyway: I'm not sure even ESP32SPI would fit on them. |
|
Test build environment needs adafruit_ticks? |
|
build fails because the test is modifying a private variable using the otherwise unused get_monotonic_time function. Either the build needs to be modified appropriately, or get_monotonic_time needs to be modified with backwards-incompatible changes. |
I'd say the function needs to be removed and the test adapted per the changes in the module. |
|
Ok, I made a couple of modifications to the test_loop file, one which is likely fine, one which I'm not sure if it's ok or not. The .loop function was returning a list of length 3 instead of 2, and I'm not sure if that's just some slight differences in timing, or it's really a failed check. After those mods, the tests passed, but the build is complaining about adafruit_ticks: File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/adafruit_ticks.py", line 31, in |
Anyhow, not related to your changes (modulo pulling the Also, I am not actually sure where does the def const(x):
"Emulate making a constant"
return x |
|
The |
|
@vladak: @jepler discussed this with @kevin-tritz in Discord. The problem I think was that |
|
Looks good. |
|
Ideally, this should be tested on a microcontroller. I may give it a go on my QtPy. |
|
I have tested this a bit on my new #!/usr/bin/env python3
import adafruit_logging as logging
import random
import socketpool
import ssl
import sys
import time
import wifi
from secrets import secrets
import adafruit_minimqtt.adafruit_minimqtt as MQTT
def on_message(client, topic, msg):
#logger = logging.getLogger(__name__)
#logger.info(f"Got msg on '{topic}' ({len(msg)} bytes)")
client.user_data[0] += 1
client.user_data[1] += len(msg)
def main():
logger = logging.getLogger(__name__)
logger.setLevel(logging.DEBUG)
logger.info("Connecting to wifi")
wifi.radio.connect(secrets["ssid"], secrets["password"], timeout=10)
logger.debug(f"IP: {wifi.radio.ipv4_address}")
pool = socketpool.SocketPool(wifi.radio)
host = "test.mosquitto.org"
port = 1883
stats = [0, 0]
mqtt_client = MQTT.MQTT(
broker=host,
port=port,
socket_pool=pool,
ssl_context=ssl.create_default_context(),
connect_retries=1,
recv_timeout=5,
use_binary_mode=True, # test.mosquitto.org has messages with UnicodeError
user_data=stats,
)
# mqtt_client.logger = logger
mqtt_client.on_message = on_message
logger.debug(f"connecting to MQTT broker {host}")
mqtt_client.connect()
topic = "#"
logger.debug(f"subscribing to {topic}")
mqtt_client.subscribe(topic)
while True:
try:
mqtt_client.loop(1)
except MQTT.MMQTTException as e:
logger.error(f"loop failed: {e}")
return
logger.info(f"Messages: {stats[0]}, bytes: {stats[1]}")
if __name__ == "__main__":
try:
main()
except KeyboardInterrupt:
sys.exit(0)and it ended up with error after 2 minutes: This is rather strange and should be investigated, however this also happens with vanilla 7.6.3, i.e. before the changes. |
|
I reduced the subscription to use the |
|
Added your suggested changes: (ticks_ms() / 1000 > ping_timeout). I assumed the MQTT Exception needed an f-string. |
Cool, looks good. Indeed, f-string is needed. |
|
@vladak We appreciate your testing. Do you think this is in good shape now? @kevin-tritz Note there is now a merge conflict to resolve. |
yep, looks good to me. |
There was a problem hiding this comment.
There is now a minor test failure. See https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/actions/runs/9370198935/job/25796562379?pr=210#step:2:805, starting at line 769. The timeout test is off by a bit (the < 8 comparison).
Yeah, saw that. Might be due to the differences that showed up in the merge conflict. I'll take a look in a bit. |
There was a problem hiding this comment.
I took I look into the failing test and pushed a fix for it.
The assertion that was failing boiled down to 8.000646777999464 < 8. Technically false, but not really a meaningful period of time.
Rounding the value to 2 decimal places and using <= instead of < allows the test to pass, and it would still fail if some part of the mechanism was broken causing unexpected delays in the timeout somewhere.
I think this looks good to go with this change.
Co-authored-by: Dan Halbert <[email protected]>
|
@dhalbert Yep, that sounds good to me. I've committed that change. |
dhalbert
changed the title
Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.10.0 from 7.9.0: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#210 from kevin-tritz/timestamp_ns Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
The code originally loses timing precision even using time.monotonic_ns because it stores timestamps after using floating point division. If the timestamps are kept as ns integers, and floating point division is only done after subtracting from the current monotonic_ns time, precision can be maintained.