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

Update to current adafruit_httpserver & make response dynamic #2

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

jepler
Copy link
Contributor

@jepler jepler commented Feb 16, 2024

Together with adafruit/circuitpython#8932 this https server is verified to work on Adafruit MatrixPortal S3 with ESP32S3 running 9.0.0-beta.0-38-g0c3b62fac2-dirty on 2024-02-16.

The bundled httpserver was old and its mpy format was not
compatible with circuitpython 9
Now it'll name the specific device and version in the returned
(text/plain) content.
Copy link
Owner

@ide ide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add requirements.txt too so that it acts as lockfile for more deterministic downloads of the httpserver module since it's not vendored anymore?

@@ -24,8 +24,12 @@ fi
echo "Found device at $device_path"

echo 'Copying application code...'
rsync --verbose --recursive --delete --checksum \
rsync --verbose --recursive --delete --checksum -@2 \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the 2 do? Also prefer to use longhand notation for flags over shorthand in scripts for self-documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This improves detecting that a file is unchanged via timestamps, and speeds up rsync as a result (though it may also need --times so that the target timestamp is set to the source timestamp).

It should be -@1 or --modify-window=1, not 2. from man rsync:

       --modify-window=NUM, -@
              When comparing two timestamps, rsync treats  the  timestamps  as
              being  equal  if  they  differ by no more than the modify-window
              value.  The default is 0, which matches  just  integer  seconds.
              If  you  specify  a negative value (and the receiver is at least
              version 3.1.3) then nanoseconds will also be taken into account.
              Specifying  1  is  useful  for  copies  to/from  MS  Windows FAT
              filesystems, because FAT represents times with a 2-second  reso‐
              lution  (allowing  times  to differ from the original by up to 1
              second).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, please use --modify-window instead of -@

scripts/deploy.sh Outdated Show resolved Hide resolved
src/code.py Outdated Show resolved Hide resolved
Co-authored-by: James Ide <ide@users.noreply.github.com>
@jepler
Copy link
Contributor Author

jepler commented Feb 18, 2024

Could you please add requirements.txt too so that it acts as lockfile for more deterministic downloads of the httpserver module since it's not vendored anymore?

I don't think circup supports installing a specific library by version, it only installs from the "latest bundle". So, the alternatives would seem to be to allow the version to be "whatever circup does", or to vendor a new version of the files. Is it OK if I update this PR to include adafruit_httpserver as 9.x mpy files instead?

@ide
Copy link
Owner

ide commented Feb 18, 2024

IME circup supports requirements.txt, take a look at circup freeze.
I'm also not opposed to vendoring the adafruit_httpserver files either. I'd just like to pin the dependencies somehow.

Copy link
Owner

@ide ide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants