prusa3d / Prusa-Firmware-Buddy

Firmware for the Original Prusa MINI, Original Prusa MK4 and the Original Prusa XL 3D printers by Prusa Research.
Other
1.14k stars 221 forks source link

[BUG] Can't connect to SSID starting with space #2325

Open Crocmagnon opened 2 years ago

Crocmagnon commented 2 years ago

Bug description

Printer type - [MINI]

Printer firmware version - 4.4.0 Original or Custom firmware - Original

Optional upgrades - Filament Runout Sensor

USB drive or USB/Octoprint N/A

Describe the bug I can't connect the Prusa Mini to my wifi network. Its SSID starts with a space.

How to reproduce

I've tried the following config entries (with and without quotes):

[wifi]
ssid= REDACTED SSID
key_mgmt=WPA
psk=REDACTED_PSK
[wifi]
ssid=" REDACTED SSID"
key_mgmt=WPA
psk="REDACTED_PSK"

Expected behavior The printer should connect to the wifi network.

I tried connecting it to another AP not starting with a space - tethering with my phone, and it works fine.

G-code N/A

Crash dump file dump.bin.zip

Additional analysis

According to the docs, the code parsing INI files strips whitespaces:

https://github.com/prusa3d/Prusa-Firmware-Buddy/blob/215bcf5b377220655c831c730940324d4dbdbf47/lib/inih/ini.h#L42-L55

Parse given INI-style file. May have [section]s, name=value pairs (whitespace stripped)

The code that strips the whitespaces from the value is here:

https://github.com/prusa3d/Prusa-Firmware-Buddy/blob/215bcf5b377220655c831c730940324d4dbdbf47/lib/inih/ini.c#L195-L196

Also, from what I can see, there is no handling of quotes to delimit strings.

If I take my two config examples:

[wifi]
ssid= REDACTED SSID
key_mgmt=WPA
psk=REDACTED_PSK

This is parsed as:

{
    "ssid": "REDACTED SSID",
    "psk": "REDACTED_PSK",
    "key_mgmt": "WPA"
}

(missing leading whitespace)

[wifi]
ssid=" REDACTED SSID"
key_mgmt=WPA
psk="REDACTED_PSK"

While that should be parsed as:

{
    "ssid": "\" REDACTED SSID\"",
    "psk": "\"REDACTED_PSK\"",
    "key_mgmt": "WPA"
}

(with quotes not part of the SSID or PSK)

Spaces in SSID are valid, as you show in the guide, but they're also valid at the beginning and and of the string. I believe the same story is true for the PSK.

I understand the choice of stripping whitespaces because it would more likely be a config error, although I believe with sufficient documentation it could be allowed. Or, we could have a config option in the printer menu "strip whitespaces while parsing INI". It would be enabled by default so the current behavior still remains the default one with the option to change it.

I'd love to try and remove the two incriminated lines and test on my MINI, but I don't have a broken tab to install custom firmware so I guess I'll wait 🙂

chillygithub commented 2 years ago

Using spaces in the ESSID or PSK is far from Best Practice. And should be avoided, as its is known to break many implementations of the wireless stack.

Crocmagnon commented 2 years ago

It may not be best practice but it’s definitely permitted by the standard 🙂 Since we’re in the process of building such an implementation, I propose supporting this behavior.

EDIT: Quick note: I’ve previously successfully connected many ESP8266 to my wifi network so the hardware shouldn’t get in the way.

laloch commented 2 years ago

Hi @Crocmagnon, thanks for reporting. While technically you're right that SSID can be any binary string including non-printable characters as long as it fits 32 octets, practically there are probably no consumer devices allowing to do so. We will certainly discuss the issue and my vote will certainly be for sticking with Cisco SSID policy which explicitly disallows leading and trailing whitespace characters.

Edit: We should, however, definitely add an option to connect using BSSID to allow for connecting to "hidden" networks and networks with unsupported SSIDs.

Crocmagnon commented 2 years ago

I guess using the BSSID would be fine! 👍🏻

practically there are probably no consumer devices allowing to do so

You’d probably be surprised to know that I never had any issue connecting devices such as laptops, phones (even slightly older models), e-readers, ESP, AC unit, robot vacuum, smart home bridges or other smart home gadgets.

This is the first time I’m getting blocked because of the space 😊 (and it’s not even because the hardware doesn’t support it, it’s only because the config file parser wants to strip spaces 😅).

In the past I also experimented with emojis, but it was widely unsupported.

I understand that leading/trailing spaces are a recipe for failure though, but I used to live in a wifi crowded place and this was an acceptable solution to “always” display my SSID first in alphabetical order (also, now I have a bunch of connected devices and no intention of repairing everything).

Crocmagnon commented 2 years ago

Hi @laloch ! Thanks to the team for the RC1 release 😀 I noticed that the release notes didn't mention the BSSID capability. Have you had time to discuss the feature?

Thanks 🙂

laloch commented 1 year ago

Hi @Crocmagnon, we have the BSSID capability in our backlog. It is, however, scheduled for 4.5.0 at the moment.

Crocmagnon commented 1 year ago

Thanks for the feedback! 👍🏻 Looking forward to testing this on my network.

nmschulte commented 1 year ago

My SSID has a trailing space and I ran into this issue as well. Has this made it into the current release, v4.7.1; this doesn't seem documented if so; can we use it now? Otherwise, what is the status of the BSSID spec issue?

Supporting BSSID would work, but also unsure why a leading or trailing space does not: it seems like the code would have to be trimming the field rather than taking it verbatim (up to the new-line), which is odd to me.

Crocmagnon commented 1 year ago

it seems like the code would have to be trimming the field

that’s what it does

nmschulte commented 1 year ago

unsure why a leading or trailing space does not: it seems like the code would have to be trimming the field rather than taking it verbatim (up to the new-line), which is odd to me.

that’s what it does

So I discovered; thanks. I understand why this is the case now as well: https://en.wikipedia.org/wiki/INI_file#Keys_(properties)

I created a PR (#3158) in attempt to supported quoted string values, but I suspect this may get rejected. Another idea is to support escape sequences, allowing to use a Unicode escape sequence for the leading/trailing spaces, but for similar reasons I feel this may be unwelcome.

lovemetwotimes commented 1 year ago

SSID with spaces inside string also doesnt work.

nmschulte commented 1 year ago

SSID with spaces inside string also doesnt work.

SSIDs with internal spaces works fine for me.

douglsmith commented 9 months ago

Thanks @nmschulte. I'm hoping your PR is implemented because I have a long-existing Wi-Fi network with a PSK that ends in a space. Changing the PSK is not really an option because it would require reconfiguring about 100 other devices and convincing others that the change is worth that pain. This would be just to accommodate one MK4 that can't deal with a character that is allowed in the Wi-Fi specifications. No other device on our network has ever had an issue with this in over a decade of use.

nmschulte commented 4 months ago

we have the BSSID capability in our backlog. It is, however, scheduled for 4.5.0 at the moment.

v4.5.0 has long since passed. #3158 has received no attention. @laloch, will this issue ever be resolved?

github-actions[bot] commented 2 months ago

This issue has been flagged as stale because it has been open for 60 days with no activity. The issue will be closed in 7 days unless someone removes the "stale" label or adds a comment.

Crocmagnon commented 2 months ago

This issue has been flagged as stale because it has been open for 60 days with no activity. The issue will be closed in 7 days unless someone removes the "stale" label or adds a comment.

Oh please no not you Prusa 😔 I can come here every now and then to comment. I can even setup a script to automatically post a useless comment regularly to keep this alive. No comment doesn’t mean the issue doesn’t exist anymore or people aren’t interested anymore. It just means that everything has been said and we’re waiting for a real implementation.

Either leave this open if you actually plan on implementing it someday or close it if you don’t but please, no stalebot 😩

danopernis commented 2 months ago

@Crocmagnon that's OK, we only needed to know if this was still active. Now that we know, I am assigning this so the stalebot won't touch it anymore.

We are planning to fix this using https://github.com/prusa3d/Prusa-Firmware-Buddy/pull/3158 but I can't give you ETA yet.

Crocmagnon commented 2 months ago

Sorry, I've been bit by organisations using stalebot to massively close issues in the past and I'm quite bitter about it. I shouldn't have reacted like this.

Thank you for your answer ❤️