umqtt.simple: Encode subscribe Remaining Length as VBI.

[pablogventura]

Summary

Fixes #969. subscribe (and unsubscribe) packed Remaining Length with
struct.pack_into("!BH", ...), so values above 127 overflow a single byte
and long topics fail on the wire.

MQTT 3.1.1 section 2.2.3 requires a Variable Byte Integer. This PR encodes
Remaining Length that way for subscribe/unsubscribe, sharing a small
_encode_len helper with connect and publish.

Testing

• Unix port: micropython micropython/umqtt.simple/test_umqtt_simple.py
  (mock socket, no broker):
  • short topic (single-byte Remaining Length)
  • subscribe with topic length 123 (Remaining Length 128 -> 82 80 01)
  • unsubscribe with topic length 124 (Remaining Length 128 -> a2 80 01)
• simple.mpy via mpy-cross vs upstream/master: 2512 -> 2515 bytes (+3).

Trade-offs and Alternatives

• Subscribe and unsubscribe share _send_subunsub; Remaining Length encoding
  is factored into _encode_len and reused by connect/publish to keep .mpy
  size close to master.
• unsubscribe is included because it had the same one-byte Remaining Length
  bug.

Generative AI

I used generative AI tools when creating this PR, but a human has checked the
code and is responsible for the code and the description above.

[dpgeorge]

Thanks for the fix.

Can you try and reduce compiled .mpy code size here? Eg by creating a helper function, or even combining subscribe and unsubscribe into one helper function.

[pablogventura]

Thanks. I combined the shared path into _send_subunsub() in 51bbbaa (subscribe/unsubscribe now just set type, ACK opcode, and optional QoS).

simple.mpy sizes with mpy-cross vs upstream/master:

Version	bytes	vs master	vs previous commit
master	2512	-	-
first fix (duplicated VBI)	2658	+146	-
_send_subunsub helper	2568	+56	-90

Unix mock tests still pass.

[pablogventura]

Thanks again. Further size pass in the latest commit: shared _encode_len() used by connect, publish, and _send_subunsub.

simple.mpy sizes with mpy-cross (compiled as simple.py in umqtt/):

Version	bytes	vs master	vs previous
master	2512	-	-
first fix (duplicated VBI)	2658	+146	-
_send_subunsub	2568	+56	-90
shared _encode_len	2515	+3	-53

Unix mock tests still pass.

[pablogventura]

Fixed the ruff E711 failure: _send_subunsub now uses ack_n > 3 instead of comparing qos to None (subscribe ACK is 4 bytes, unsubscribe is 3).

.mpy size unchanged: 2515 bytes (+3 vs master).

[dpgeorge]

See #303 for a previous attempt at this. Is there anything from that PR that's missed in this PR here?

[pablogventura]

Thanks for the pointer to #303.

I compared it against this PR. For the Remaining Length bug itself (#284 / #969), nothing essential from #303 is missing here: subscribe now uses a Variable Byte Integer, and this PR also fixes unsubscribe (that method did not exist yet when #303 was opened).

#303 was a broader refactor on top of the bug fix: shared _varlen_encode, replacing struct with int.to_bytes / from_bytes, reshaping the CONNECT fixed/variable header layout, optional password when username is set, larger packet buffers / assert up to 2^28, plus example CLI tweaks and a debug wrapper. I left those out on purpose to keep the diff small and stay close to the current publish/connect patterns.

On size, compiling simple.py with mpy-cross against current master:

Version	bytes	vs master
master	2512	-
this PR	2515	+3
#303-style helper + sub/unsub fix on today's master	~2575-2593	+63 to +81

So the focused approach here is substantially smaller than porting the #303 refactor onto the current tree. Happy to open a follow-up later for any of the non-bug items from #303 if you want them (e.g. username without password).

[dpgeorge]

Thanks for investigating #303 in detail. I agree the approach here looks better: smaller and covers the new unsubscribe() method.

I noticed the new test is not running under CI. Please enable it using the following:

--- a/tools/ci.sh
+++ b/tools/ci.sh
@@ -57,6 +57,7 @@ function ci_package_tests_run {
     export MICROPYPATH
     for test in \
         micropython/drivers/storage/sdcard/sdtest.py \
+        micropython/umqtt.simple/test_umqtt_simple.py \
         micropython/xmltok/test_xmltok.py \
         python-ecosys/requests/test_requests.py \
         python-stdlib/argparse/test_argparse.py \

After that's done, this PR should be good to merge.

[pablogventura]

Thanks. Added micropython/umqtt.simple/test_umqtt_simple.py to ci_package_tests_run in tools/ci.sh as suggested (eecbe36).

Verified locally with the same invocation CI uses: cd micropython/umqtt.simple && micropython test_umqtt_simple.py.

[dpgeorge]

Merged!

Thanks for your work on this, it's a good improvement. And nice that the code size impact was almost zero.

[pablogventura]

Thanks for merging, and for the careful review throughout.

I'm glad to contribute to micropython and happy to keep helping. If there is any area or issue you would particularly like me to focus on next, just point me at it.