Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"noReset" toggle is not working in published version #303

Closed
dhalbert opened this issue Nov 15, 2024 · 21 comments
Closed

"noReset" toggle is not working in published version #303

dhalbert opened this issue Nov 15, 2024 · 21 comments
Assignees

Comments

@dhalbert
Copy link
Contributor

@mikeysklar @makermelissa

(Found while helping a user in discord: see https://discord.com/channels/327254708534116352/537365702651150357/1306827591407239248 and preceding)

#297 by @mikeysklar added a toggle to suppress trying to send a reset, for use with passthrough updaters like on PyPortal. But it is not working in the published version.

I tested the tip of main locally, which includes #297, and it works fine. If the toggle is on, I see:

ESP Web Flasher loaded.
Connecting...
Connected successfully.
Try hard reset.
No reset requested; skipping hard reset.
...

But the published version at https://adafruit.github.io/Adafruit_WebSerial_ESPTool/ does not work. It has the toggle, but it does not print No reset requested; skipping hard reset. This makes me think that the code checking the status of the checkbox is not working for some reason in the published version. Maybe the document.getElementById() is not working?? I'm not sure how to test this locally.

    // Check for noReset toggle
    const noResetCheckbox = document.getElementById("noReset");
    const noResetEnabled = noResetCheckbox
      ? (noResetCheckbox as HTMLInputElement).checked
      : false;

    if (noResetEnabled) {
      this.logger.log("No reset requested; skipping hard reset.");
      return; // Skip reset if noReset is enabled
    }
@mikeysklar
Copy link
Contributor

Thanks @dhalbert ... I'll work on this on Friday.

@mikeysklar
Copy link
Contributor

Wired up a PyPortal and reproduced @dhalbert results.

  • local version runs
  • published version ignores "Turn on for passthrough updates" and tries to reset

I'll try observing the browser interaction and maybe setting up a matchy matchy web server on my own host (not just using localhost) to see if I can reproduce the production code behavior.

@mikeysklar
Copy link
Contributor

mikeysklar commented Nov 15, 2024

I did some F12 browser debugging and see an issue with my code from the last PR.

js/script.js

/**
 * @name clickNoReset
 * Change handler for ESP32 co-processor boards
 */
async function clickNoReset() {
  saveSetting("noReset", noReset.checked);
  if (espStub) {
    try {
      // Assuming espStub has a setNoReset method, similar to setBaudrate
      await espStub.setNoReset(noReset.checked);
    } catch (error) {
      console.error("Failed to set noReset:", error);
    }
  }
}

The espStub does not have a setNoReset. I'm not sure how the code was working locally without it. A setNoReset needs to be in the src/espload.ts.

@mikeysklar
Copy link
Contributor

mikeysklar commented Nov 15, 2024

Well the PR #304 seemed promising, but with the on-line version still ignoring the added hardreset() checkbox logic I'll need to create a working dev server.

I think I can enable github.io to work on my fork of the code so we don't need to be messing with the production version.

@tyeth
Copy link

tyeth commented Nov 16, 2024 via email

@mikeysklar
Copy link
Contributor

@tyeth - Thank you so much for pointing these two items out.

I see the jsdeliver link is pointing at an earlier release that did not support passthrough.

https://cdn.jsdelivr.net/gh/adafruit/Adafruit_WebSerial_ESPTool@1.2.0/dist/web/index.js

I just made a change to point it at the 1.3.0 current release with passthrough.

@dhalbert
Copy link
Contributor Author

It's working now, in the deployed version! Thank you @tyeth @mikeysklar.

This seems like a bit of a catch-22. The version link needs to be updated in advance of doing the actual release. So ideally there would be a 1.3.1 release with #304 included, but we also need another PR that points to 1.3.1.

@mikeysklar
Copy link
Contributor

mikeysklar commented Nov 16, 2024

Confirmed, also works for me. Really needed to @tyeth to point out that hardcoded version number.

Dan,

I can submit another update that uses something like @latest, @main or even @master.

const scriptUrl = "https://cdn.jsdelivr.net/gh/adafruit/Adafruit_WebSerial_ESPTool@latest/dist/web/index.js";

What do you think?

@dhalbert
Copy link
Contributor Author

const scriptUrl = "https://cdn.jsdelivr.net/gh/adafruit/Adafruit_WebSerial_ESPTool@latest/dist/web/index.js";

That is great. I did not realize that would work -- I thought it had to be a release. That is much better than choosing a release number, making a PR, and then having to make a release immediately.

@mikeysklar
Copy link
Contributor

mikeysklar commented Nov 16, 2024

Do you have a preference between:

  1. @latest
  2. @main
  3. @master

@dhalbert
Copy link
Contributor Author

There is no @master. I think @latest makes sense, but I think it's the same as @main, since @main is the main branch.

@mikeysklar
Copy link
Contributor

kk...PR #306 with @latest is in

@mikeysklar
Copy link
Contributor

@thx. Test on Feather V2 and PyPortal web release looks good.

@tyeth
Copy link

tyeth commented Nov 16, 2024 via email

@dhalbert
Copy link
Contributor Author

Note that @thx, @latest, @main, @master are all tagging usernames on GitHub. 🙂

@tyeth
Copy link

tyeth commented Nov 16, 2024 via email

@dhalbert
Copy link
Contributor Author

Fixed by #304, #305, #306.

@dhalbert
Copy link
Contributor Author

@tyeth so do you use a version number, and make sure to do a release at the same time?

@tyeth
Copy link

tyeth commented Nov 16, 2024

I think in another repo I was trying to avoid having to release each change, and tags was enough for it to pickup quickly using branch name or latest, but only a new tag in cdn URL was quick enough to get me immediate updates (due to not being cached yet by jsdelivr)

@tyeth
Copy link

tyeth commented Nov 16, 2024

@dhalbert see above, sorry I'm bad at pinging, and here was the script I was using. Effectively I had vscode open and was rapidly saving, toggling to terminal and updating with this script then toggling to chrome to test (when local was done with and wanted to fix my jsdelivr version which behaved differently than local) https://github.com/tyeth/serialfruit-connect-bookmarklet/blob/main/update_refs.py#L69-L77

@dhalbert
Copy link
Contributor Author

No need to ping me -- I am subscribed to the issue (I get email for all adafruit repos, and I look at it).

I think if there's a short delay in general that's fine. When I merged the PR yesterday it showed up on the CDN almost immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants