Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) #2059

sidprice · 2025-01-19T21:43:51Z

This PR adds Wi-Fi management for ctxLink using GDB and a new monitor command.

With the original releases of ctxLink some users had issues using the HTTP and WPS provisioning features of ctxLink.

Also, after provisioning some users found it hard to discover the IP address of ctxLink, this is required to connect to the probe.

The new monitor command is "wifi".

To inspect the current Wi-Fi status the following command is used:

mon wifi

This will display either "Not Connected," or the SSID, Signal Strength (RSSI), and IP address of the connected Access Point.

To connect to a Wi-Fi Access Point the following command is used, substituting the SSID and PassPhrase settings of the AP:

mon wifi SSID,Passphrase

Spaces are accepted for these two parameters, however, there should be no space before or after the ",".

Your checklist for this pull request

I've read the Code of Conduct
I've read the guidelines for contributing to this repository
It builds for hardware native (see Building the firmware)
It builds as BMDA (see Building the BMDA)
I've tested it to the best of my ability
My commit messages provide a useful short description of what the commits do

Closing issues

None.

Misaka0x2730 · 2025-02-12T12:17:06Z

Hi!
First off, apologies if I’m stepping on your toes—I don’t mean to meddle in something that might be out of my scope.
I just wanted to suggest that there might be another way to achieve what you’re aiming for without modifying command.c.
Please, check the platform's custom command feature (PLATFORM_HAS_CUSTOM_COMMANDS), and you can find an example in my project here: https://github.com/Misaka0x2730/MioLink/blob/main/firmware/source/bmp/platform_commands.c#L38C17-L38C34
It could help keep the core code untouched while still delivering the same functionality.

sidprice · 2025-02-12T15:46:42Z

@Misaka0x2730 Hello!

This was previously suggested by @ALTracer, and is a good idea. Since this PR is a feature update it is awaiting review and merging until after the next release. Hopefully, during this period I can adopt the suggestion.

sidprice · 2025-02-12T18:21:53Z

I am not sure this is such a good idea since it prevents grouping the custom commands logically in the command list. Plus, there are quite a few other custom commands already in the command list, e.g. TraceSWO & RTT.

I think unless the maintainers (@dragonmux ) feel it is required I am going to leave the added command for this PR as it is.

Misaka0x2730 · 2025-02-13T06:34:28Z

I agree that @dragonmux is the one who’ll ultimately decide here.
However, I want to clarify my thoughts on “custom” commands. For instance, TraceSWO and RTT are supported by many (if not most) platforms—so they’re hardly “custom” in the sense of belonging to just one.
By contrast, Wi-Fi is specific to your platform, which I believe aligns more with the idea of platform-specific commands.

perigoso · 2025-02-13T12:19:01Z

I agree that these should be platform commands at least for now, though not having control over the grouping is unfortunate.
I have some ideas to rework to the command system but until then we are limited with what we can do. Do note that this only affects the print on the help command and doesn't affect functionality

sidprice · 2025-02-13T14:53:21Z

@perigoso Thanks for your input. I would be happy to review the platform commands should the command table/architecture change in the future.

…d save for display to user.

Display is incorrect, needs to treat input bytes as hex.

This is required to keep the wi-fi stack operational, the BMF code blocks in this function at times and therefore, wi-fi is not serviced.

…nfo.

dragonmux · 2025-02-24T17:08:50Z

Having had a conversation with Sid on Discord, we are switching this to being a fix PR rather than a feature, and promoting it into the v2.0 track for after rc2's release.

dragonmux

Apologies getting to review this took so long. Most of the items are types related (signed-unsigned conversions mostly) and should all be pretty simple fixes. With them taken care of we'll get this merged so this issue can be fixed for the full v2.0 release.

dragonmux · 2025-03-02T04:18:43Z

src/command.c

 #ifdef PLATFORM_HAS_BATTERY
 static bool cmd_target_battery(target_s *t, int argc, const char **argv);
 #endif
+#ifdef PLATFORM_HAS_WIFI
+static bool cmd_wifi(target_s *t, int argc, const char **argv);
+#endif


Not for this PR, but for a future one: These should probably both become platform commands. That way you don't need to export as much platform state into the rest of the code base, and they get grouped under the platform commands heading in the mon help output. We can look at improving this in v2.1 though so this is fine as is for now.

Agreed as previously discussed.

src/platforms/ctxlink/platform.c

src/platforms/ctxlink/ctxLink_mode_led.c

dragonmux · 2025-03-02T04:23:08Z

src/platforms/ctxlink/WiFi_Server.c

@@ -531,6 +536,12 @@ static void app_wifi_callback(uint8_t msg_type, void *msg)
 		break;
 	}

+	case M2M_WIFI_CONN_INFO_RESPONSE_EVENT: {
+		tstrM2MConnInfo *connection_info = (tstrM2MConnInfo *)msg;
+		memcpy(&conn_info, connection_info, sizeof(tstrM2MConnInfo));


For what it's worth, memcpy() takes a void pointer for the pointer parameters so you could simplify this to memcpy(&conn_info, msg, sizeof(tstrM2MConnInfo)); and forgo the pointer conversion and local.

Agreed, however the local aids with debugging, that is why it was added.

Hmm.. ah, by allowing you to view the pre-and-post-image of the memcpy() before running it?

Not really, the parameter "msg" is a void pointer from the WINC1500 stack and it may point to one of several different structures depending upon the message type (msg_type). I use an intermediate local of the expected type to allow easier examination of the structure with the debugger. I think originally, the debugger didn't handle type casting too well, hence this approach.

dragonmux · 2025-03-02T04:24:24Z

src/platforms/ctxlink/WiFi_Server.c

@@ -558,10 +569,14 @@ static void app_wifi_callback(uint8_t msg_type, void *msg)
 	}

 	case M2M_WIFI_IP_ADDRESS_ASSIGNED_EVENT: {
-		t_wifiEventData *wifi_event_data = (t_wifiEventData *)msg;
-		if (wifi_event_data != NULL)
+		tstrM2MIPConfig *ip_config = (tstrM2MIPConfig *)msg;


Beyond checking the pointer is non-NULL, is this local used for anything here? If not then the if should probably become if (msg != NULL)

Like the above comment, the local is a useful debugging addition.

src/platforms/ctxlink/WiFi_Server.c

Required for new network "forget" command.

Embedded spaces not being processed correctly.

dragonmux

This is looking much better - good work! Spotted a couple of small things in the rewritten wifi_connect() logic, but with them taken care of we're happy to merge this.

src/platforms/ctxlink/WiFi_Server.c

Previous version of the parser did not handle multiple spaces.

dragonmux

LGTM, merging - nice work cleaning things up so quickly. Thank you for the contribution!

sidprice force-pushed the ctxlink_wifi_options branch 3 times, most recently from 758729a to 4e6ea5c Compare January 19, 2025 22:26

dragonmux added this to the v2.1 release milestone Jan 20, 2025

dragonmux added Enhancement General project improvement Foreign Host Board Non Native hardware to runing Black Magic firmware on labels Jan 20, 2025

sidprice force-pushed the ctxlink_wifi_options branch from 4e6ea5c to 791891e Compare January 20, 2025 15:49

sidprice force-pushed the ctxlink_wifi_options branch 3 times, most recently from 447e76d to eabddff Compare February 8, 2025 17:40

sidprice added 13 commits February 13, 2025 11:52

ctxlink/command: Add skeleton for new 'wi-fi' monitor command.

34694d2

ctxlink/wifi_server: Add code to collect the ip address of ctxlink an…

aad7f5e

…d save for display to user.

ctxlink/wifi_server: Request and save the wifi connection data.

347a430

ctxlink/command: Prototype report of IP address

bc6d523

Display is incorrect, needs to treat input bytes as hex.

ctxlink/wifi_server: Add reporting of Wi-Fi parameters

2333d41

ctxlink/wifi: Update new command help string.

8401337

ctxlink/wifi: Fix bug in network display

102fe3d

ctxlink/platform: Add polling of platform tasks to gdb_usb_getchar

fcd6ae5

This is required to keep the wi-fi stack operational, the BMF code blocks in this function at times and therefore, wi-fi is not serviced.

ctxlink/wifi_server: Add function to request current network information

38cca08

ctxlink/wifi_server: Implement state machine updates for connection i…

aafe9a3

…nfo.

ctxlink/platform: Implement monitor command to set network parameters.

5cf8bdc

ctxlink/wifi_server: Add timeout to network connect

0dcbedc

ctxlink/wifi_server: Allow Wi-Fi connection to any (2g) channel.

0358831

sidprice force-pushed the ctxlink_wifi_options branch from eabddff to 11b0093 Compare February 13, 2025 18:53

dragonmux changed the title ~~Ctxlink_wifi_options~~ Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) Feb 24, 2025

dragonmux added the Bug Confirmed bug label Feb 24, 2025

dragonmux modified the milestones: v2.1 release, v2.0 release Feb 24, 2025

dragonmux requested changes Mar 2, 2025

View reviewed changes

sidprice added 7 commits March 2, 2025 17:45

ctxlink/ctxlink_mode_led: Fix enum value.

0bf3444

ctxlink/wifi_server: Add wait for disconnect to app_task.

6150b4f

Required for new network "forget" command.

ctxlink/platform: Add network "forget" command in monitor

24759e0

ctxlink/platform: Correct monitor command help text.

6eed6fa

ctxlink/wifi_server: Add function to read winc1500 firmware release

e475e7f

ctxlink/wifi_server: Fix bug in parsing SSID and Pass Phrase

05e3d8c

Embedded spaces not being processed correctly.

ctxlink/wifi: Remove unwanted monitor wifi options.

938f421

sidprice force-pushed the ctxlink_wifi_options branch from 11b0093 to af71d57 Compare March 3, 2025 22:10

dragonmux reviewed Mar 4, 2025

View reviewed changes

src/platforms/ctxlink/WiFi_Server.c Outdated Show resolved Hide resolved

src/platforms/ctxlink/WiFi_Server.c Outdated Show resolved Hide resolved

src/platforms/ctxlink/WiFi_Server.c Outdated Show resolved Hide resolved

ctxlink/wifi_server: Rework SSID and Password parsing

bf9529a

Previous version of the parser did not handle multiple spaces.

sidprice force-pushed the ctxlink_wifi_options branch from af71d57 to bf9529a Compare March 4, 2025 16:45

dragonmux approved these changes Mar 4, 2025

View reviewed changes

dragonmux merged commit bf9529a into blackmagic-debug:main Mar 4, 2025
36 checks passed

sidprice deleted the ctxlink_wifi_options branch March 4, 2025 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) #2059

Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) #2059

sidprice commented Jan 19, 2025

Misaka0x2730 commented Feb 12, 2025

sidprice commented Feb 12, 2025

sidprice commented Feb 12, 2025

Misaka0x2730 commented Feb 13, 2025

perigoso commented Feb 13, 2025

sidprice commented Feb 13, 2025

dragonmux commented Feb 24, 2025

dragonmux left a comment

dragonmux Mar 2, 2025

sidprice Mar 2, 2025

dragonmux Mar 2, 2025

sidprice Mar 2, 2025

dragonmux Mar 4, 2025

sidprice Mar 4, 2025

dragonmux Mar 2, 2025

sidprice Mar 2, 2025

dragonmux left a comment

dragonmux left a comment

Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) #2059

Fix: WiFi configuration issues via USB (HTTP/WPS provisioning sometimes unreliable) #2059

Conversation

sidprice commented Jan 19, 2025

Your checklist for this pull request

Closing issues

Misaka0x2730 commented Feb 12, 2025

sidprice commented Feb 12, 2025

sidprice commented Feb 12, 2025

Misaka0x2730 commented Feb 13, 2025

perigoso commented Feb 13, 2025

sidprice commented Feb 13, 2025

dragonmux commented Feb 24, 2025

dragonmux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragonmux left a comment

Choose a reason for hiding this comment

dragonmux left a comment

Choose a reason for hiding this comment