Gentle request to update circuitdojo/ncs-serial-modem fork?

egnor

The circuitdojo fork of Nordic’s serial modem (https://github.com/circuitdojo/ncs-serial-modem) was last updated in June and is missing some significant updates. Of interest to me (since I use MQTT):

https://github.com/nrfconnect/ncs-serial-modem/commit/8d545f66516aea703e933e97d9442baa5507cad1 - allow AT#XMQTTPUB to give a counted payload length, otherwise you have to mess around with +++ type terminators which can’t be easily escaped
https://github.com/nrfconnect/ncs-serial-modem/commit/7c1cb929e417f22ec5396f5733e591da00c26006 - among other refactors, fixes a major regression where incoming MQTT messages would get garbled in the wrong order (#MQTTMSG: ... would come after the message topic and body)
Realistically, we could probably upstream some of the smaller tweaks, which seem like straight up correctness fixes on their side, and then it’s easy enough to add the overlay and board files without needing to fork the repo at all?

jaredwolff

Sure I’ll take a look. Thanks for the nudge @egnor

jaredwolff

It builds with the latest. Give it a go @egnor

egnor

That was quick!! Will try it out

egnor

Seems to work in an initial smoke test jaredwolff ! Thank you very much 🙏

I noticed the tweak to UART assignment (I’d had to deal with that myself).

Question for you @jaredwolff - if I were to peel off pieces of the changes this fork carries and take them to Nordic for upstreaming (to simplify this fork, maybe eventually to nothing!), are there any objections / sensitivities I should be aware of? I’ve had some luck getting PRs accepted into the SM upstream (eg. the MQTT command fix), but don’t want to go behind your back.

jaredwolff

@egnor I can put in a PR and see if it’s something they’re willing to roll with. Since it’s not an official Nordic board I think you have a 50% chance of them going for it.

jaredwolff

Actually looking at it more, I highly doubt they’ll take it since it imports extra dependencies (3.4.x version of nfed etc). You’re welcome to try and get whatever you want merged in if you can.

egnor

FYI, since it looks like both of us independently tripped over the UART-collision problem, I opened this PR upstream to make the failure a little more obvious: https://github.com/nrfconnect/ncs-serial-modem/pull/424

egnor

@jaredwolff I was going to peel off this change of yours, which looked cleanly separable, to upstream:

(file: app/sysbuild/CMakeLists.txt)

+# The same variant configure step otherwise falls back to /usr/bin/python, which
+# may not have Zephyr's Python dependencies installed. Pin it to west's Python.
+if(DEFINED WEST_PYTHON)
+  set(Python3_EXECUTABLE "${WEST_PYTHON}" CACHE FILEPATH "Python for child images" FORCE)
+  get_filename_component(WEST_PYTHON_DIR "${WEST_PYTHON}" DIRECTORY)
+  set(ENV{PATH} "${WEST_PYTHON_DIR}:$ENV{PATH}")
+endif()

what was the actual thing that broke here? In my environment, python comes from the Nordic toolchain setup (.../toolchains/fbf7391cab/usr/local/bin/python, which nrfutil sdk-manager toolchain launch --ncs-version=v3.4.0 --shell prepends to my $PATH).

Since I’m a CLI dinosaur, I don’t use the Circuit Dojo Zephyr Tools or nRF Connect for VSCode, do those do things differently such that the system python ends up first on $PATH? Or does something funny happen in the depths of Nordic cmakery that resets this to a literal /usr/bin/python3? Either way, I can’t figure out how to repro the problem which I’d want to do before proposing a fix. Even with that change commented out, I see this:

   ****************************************
   * Running CMake for mcuboot_s1_variant *
   ****************************************

Loading Zephyr default modules (Zephyr base).
-- Application: /home/egnor/source/blub/dev.tmp/nordic/workspace/bootloader/mcuboot/boot/zephyr
-- CMake version: 4.2.1
-- Found Python3: /home/egnor/source/blub/dev.tmp/nordic/toolchains/fbf7391cab/usr/local/bin/python (found suitable version "3.12.4", minimum required is "3.12") found components: Interpreter
...

…which seems like it’s picking up the python from $PATH just fine??

jaredwolff

We setup a venv for the vscode extension. If there’s multiple pythons on the PATH things break real fast. Anything I did in that repo is targeted toward getting CD Zephyr Tools working. YMMV

egnor

jaredwolff Gotcha. Filed https://github.com/zephyrproject-rtos/zephyr/pull/118182, we’ll see if they take it.

egnor

FYI this PR merged upstream! https://github.com/zephyrproject-rtos/zephyr/pull/118182
So the Python3_EXECUTABLE pass-on is at least one teensy little change hopefully you don’t have to carry going forward (whenever you do another pull, which there’s no immediate need for).

egnor

btw @jaredwolff as far as I can tell the BOARD_ROOT pass-on in the patch you carry is unnecessary at the current state of merge? Unlike the Python one this seems to get carried along in the variant flow just fine. But I could easily be missing something that happens in the GUI case.

jaredwolff

There was a reason for it. When I first started working on it there was probably a nrf sdk reason for it but they have since resolved it. This is the first time I’ve looked at it in a while. If it turns out it’s not needed I’ll nix it next time I’m in there.