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.16k stars 229 forks source link

[BUG] MK4 Network UI shows LAN as `[stat]` instead of `[static]` when the menu is re-entered #3155

Closed gudnimg closed 22 hours ago

gudnimg commented 1 year ago

Printer type - MK4 Printer firmware version - 4.7.1 Original or Custom firmware - Original USB drive or USB/Octoprint - N/A Describe the bug Missing character in the LAN type setting.

How to reproduce

  1. Go to Network settings
  2. Go to Wifi settings
  3. Change LAN from DHCP to Static --> Note the UI shows [static]
  4. Leave Wifi settings menu
  5. Enter Wifi settings menu and observe UI bug --> UI shows [stat]

IMG_3748

Expected behavior UI should show [static] always instead of [stat]

BastionNtB commented 1 year ago

I'm seeing this on 5.0.0.

Also can't seem to get a static assignment to work either.

github-actions[bot] commented 5 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.

gudnimg commented 5 months ago

Just checked using the latest MK4 firmware 6.0.1, the bug is still there:

IMG_5206

github-actions[bot] commented 2 months ago

Thank you for your contribution to our project. This issue has not received any updates for 60 days and may be considered "stale." If this issue is still important to you, please add an update within the next 7 days to keep it open. Administrators can manually reopen the issue if necessary.

gudnimg commented 2 months ago

Likely still an issue, I will wait for FW 6.1 for MK4 and look into submitting a PR. The fix is trivial if I remember correctly.

Ro3Deee commented 2 months ago

17241269052395098656850621452469 Confirmed on prusa xl

danopernis commented 2 months ago

@gudnimg yep, still there, even in 6.1.x series :slightly_frowning_face:

bkerler commented 2 months ago

I just had a look at the bug isn't "trivial" to fix at first sight. It is related to the refresh of the WI_SWITCH_t display class as it seems : https://github.com/prusa3d/Prusa-Firmware-Buddy/blob/f5a498ab8d2a42341d0dbeb969b7ae047783e860/src/gui/MItem_network.cpp#L176 I assume that the length of "DHCP" is used being the first option in the list although the length of "static" should be used.

gudnimg commented 2 months ago

If I recall correctly, then doing something similar as here:

MI_NET_INTERFACE_t::MI_NET_INTERFACE_t()
    : WI_SWITCH_t(0, _(label), nullptr, is_enabled_t::yes, is_hidden_t::no, string_view_utf8::MakeCPUFLASH((const uint8_t *)str_eth), string_view_utf8::MakeCPUFLASH((const uint8_t *)str_wifi)) {
    if (netdev_get_active_id() == NETDEV_ESP_ID) {
        this->SetIndex(1);
    } else {
        this->SetIndex(0);
    }
}

was the trick. SetIndex would trigger a recalculation of the width. Its been a long time since I tested this so take it with a grain of salt.

bkerler commented 2 months ago

Yes, that would be a valid "quick fix", but it doesn't seem to solve the actual real issue I think :) The class caller itself does call SetIndex but with the wrong index.

bkerler commented 2 months ago

I just wrote a PR #4176 that solves the issue. Not saying that I'm happy, but it does fix the issue.

MI_NET_IP::MI_NET_IP(NetDeviceID device_id)
    : WI_SWITCH_t(0, string_view_utf8::MakeCPUFLASH(label), nullptr, is_enabled_t::yes, is_hidden_t::no, string_view_utf8::MakeCPUFLASH(str_DHCP), string_view_utf8::MakeCPUFLASH(str_static))
    , device_id(device_id) //
{
    index = netdev_get_ip_obtained_type(this->device_id());
    if (index == NETDEV_STATIC) {
        this->SetIndex(1);
    } else {
        this->SetIndex(0);
    }
    // SetIndex doesn't update the displayed string length if idx==index, so we update it manually
    this->changeExtentionWidth();
}

Even though I expected that setting the Index accordingly (1 for static, 0 for dhcp), the internal index is already set and thus changeExtentionWidth isn't executed. By running it manually, it does update the string correctly. I will have a closer look why idx==index as this solves the issue but I'm not yet happy about having to manually run changeExtentionWidth to update the string length. Will discuss with the devs.

bkerler commented 2 months ago

Ok, the bug went deeper as I feared but I was able to resolve it now. The problem is, that the MI_NET_IP was using a parameter "index" which wasn't defined locally but globally, setting as well the index value of WI_SWITCH_t .. which was ... really bad.

github-actions[bot] commented 1 week ago

Thank you for your contribution to our project. This issue has not received any updates for 60 days and may be considered "stale." If this issue is still important to you, please add an update within the next 7 days to keep it open. Administrators can manually reopen the issue if necessary.

github-actions[bot] commented 22 hours ago

This issue has been closed due to lack of recent activity. Please consider opening a new one if needed.