feat: upgrade platform and support webdav#1047
feat: upgrade platform and support webdav#1047daveallie merged 6 commits intocrosspoint-reader:masterfrom
Conversation
- Switch platform from espressif32 6.12.0 to pioarduino 55.03.37
(Arduino Core 3.3.7, ESP-IDF 5.5.2)
- WiFiClient/WiFiClientSecure -> NetworkClient/NetworkClientSecure
- WiFiUDP -> NetworkUDP
- BatteryMonitor: replace removed esp_adc_cal API with analogReadMilliVolts()
- Add missing #include <string> in BaseTheme.h
Implements RFC 4918 WebDAV Class 1 server allowing the device's SD card to be mounted as a network drive in Windows Explorer, macOS Finder, Linux Nautilus, and other WebDAV clients. Supported methods: OPTIONS, PROPFIND, GET, HEAD, PUT, DELETE, MKCOL, MOVE, COPY, LOCK, UNLOCK
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new WebDAVHandler implementing WebDAV methods and integrates it into the web server; replaces WiFi-specific network types with abstract NetworkClient/NetworkClientSecure/NetworkUDP; updates platform dependency source and a header include; updates an open-x4-sdk submodule reference. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebDAV Client
participant WebServer
participant WebDAV as WebDAVHandler
participant FS as File System
participant Network as Network Layer
Client->>WebServer: HTTP/WebDAV request
WebServer->>WebDAV: canHandle()/handle() (includes headers)
WebDAV->>WebDAV: route to specific method handler
alt PROPFIND
WebDAV->>FS: list directory & stat entries
FS-->>WebDAV: metadata
WebDAV->>WebServer: multistatus XML response
else PUT
WebDAV->>WebServer: read request body stream
WebServer-->>WebDAV: data chunks
WebDAV->>FS: write temp file -> rename
WebDAV->>Network: invalidate caches if needed
else MOVE/COPY/DELETE
WebDAV->>FS: perform copy/move/delete with checks
end
WebDAV->>WebServer: send HTTP response
WebServer-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitmodulesopen-x4-sdkplatformio.inisrc/components/themes/BaseTheme.hsrc/network/CrossPointWebServer.cppsrc/network/CrossPointWebServer.hsrc/network/HttpDownloader.cppsrc/network/HttpDownloader.hsrc/network/WebDAVHandler.cppsrc/network/WebDAVHandler.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/network/HttpDownloader.cpp (2)
lib/KOReaderSync/KOReaderSyncClient.cpp (2)
isHttpsUrl(28-28)isHttpsUrl(28-28)src/util/UrlUtils.cpp (2)
isHttpsUrl(5-5)isHttpsUrl(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (16)
src/components/themes/BaseTheme.h (1)
6-6: LGTM!Adding the explicit
#include <string>is the correct fix. The file usesstd::stringin multiplestd::functionsignatures, and relying on transitive includes is fragile—especially after a major platform/toolchain upgrade where header dependencies may change.platformio.ini (1)
8-8: Platform upgrade to pioarduino 55.03.37 is correct.The pinned version ensures reproducible builds and aligns with the PR objective of upgrading to Arduino Core 3.3.7 (ESP-IDF 5.5.2).
open-x4-sdk (1)
1-1: Submodule pointer update looks fine.This file only updates the submodule SHA; no local logic to review here.
src/network/HttpDownloader.h (1)
7-10: LGTM for the updated header comment.src/network/CrossPointWebServer.h (1)
6-12: LGTM for the member wiring updates.Also applies to: 74-82
src/network/CrossPointWebServer.cpp (2)
162-167: LGTM for the WebDAV header collection and handler registration.
511-512: LGTM.src/network/HttpDownloader.cpp (1)
16-25: LGTM for the updated client selection logic.Also applies to: 67-75, 116-117
src/network/WebDAVHandler.h (1)
6-43: LGTM for the WebDAV handler interface.src/network/WebDAVHandler.cpp (7)
23-124: LGTM for RequestHandler routing.
128-136: LGTM for OPTIONS/PROPFIND responses and XML framing.Also applies to: 140-228, 230-261
265-332: LGTM for GET/HEAD handling.
359-408: LGTM for DELETE handling.
412-448: LGTM for MKCOL validation.
608-634: LGTM for dummy LOCK/UNLOCK compatibility.
638-780: LGTM for path normalization and MIME helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitmodules:
- Around line 3-4: Replace the personal fork URL and temporary branch in the
.gitmodules entry: change the url value from
"https://github.com/dexif/community-sdk.git" to
"https://github.com/open-x4-epaper/community-sdk.git" and, if/when the upstream
PR is merged, update the branch value (currently "feat/arduino-core-3x-compat")
to the official branch or tag provided by the upstream repo; ensure the
.gitmodules url and branch lines are updated together so submodule init/update
uses the official upstream source.
In `@src/network/WebDAVHandler.cpp`:
- Around line 452-514: Add an early no-op guard in WebDAVHandler::handleMove
(and likewise in WebDAVHandler::handleCopy) right after obtaining srcPath =
getRequestPath(s) and dstPath = getDestinationPath(s) (and overwrite =
getOverwrite(s)) to detect when the resolved destination equals the source; if
srcPath == dstPath (or both normalize to the same string) return a successful
no-op response (e.g., s.send(204) for MOVE) and do not perform
delete/rename/copy operations to avoid data loss.
- Around line 49-106: The handler currently deletes an existing file at
RAW_START which risks permanent data loss if the PUT fails or is aborted; modify
WebDAVHandler::raw to write to a temporary path instead and only replace the
original on successful completion: store the target path in _putPath and build a
unique temp path _putTempPath at RAW_START (do not call Storage.remove on the
original), open _putFile for writing to _putTempPath, set _putExisted based on
_putPath, stream writes as now into _putFile while checking _putOk, and on
RAW_END close _putFile then atomically replace the original by removing the
original and renaming/moving _putTempPath to _putPath (or use Storage.remove
then Storage.rename/move if available); on RAW_ABORTED close and remove
_putTempPath and set _putOk = false. Ensure all branches reference the new
members (_putPath, _putTempPath) and avoid deleting the original until the PUT
is fully successful.
.gitmodules
Outdated
| url = https://github.com/dexif/community-sdk.git | ||
| branch = feat/arduino-core-3x-compat |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .gitmodulesRepository: crosspoint-reader/crosspoint-reader
Length of output: 240
🏁 Script executed:
# Check PR status - try with gh command
gh pr view 21 --repo open-x4-epaper/community-sdk --json state,title,mergeable,url 2>&1 || echo "gh command may not have credentials"Repository: crosspoint-reader/crosspoint-reader
Length of output: 213
🌐 Web query:
open-x4-epaper/community-sdk pull request 21 arduino core compatibility
💡 Result:
Pull request #21 in open-x4-epaper/community-sdk is titled “feat: Update BatteryMonitor for Arduino ESP32 Core 3.x compatibility” and updates the BatteryMonitor library to work with Arduino ESP32 Core 3.x (ESP-IDF 5.x), where the esp_adc_cal_* ADC calibration API was removed. Instead, it switches to analogReadMilliVolts(), which performs calibration internally. [1]
Concretely, the commit:
- Removes
#include <esp32-hal-adc.h>and#include <esp_adc_cal.h>and adds#include <Arduino.h>. [2] - Changes
readRawMillivolts()to returnanalogReadMilliVolts(_adcPin)(millivolts directly). [2] - Updates
readMillivolts()to scale that millivolt reading by the divider multiplier (no separate raw→mV conversion step). [2] - Keeps
millivoltsFromRawAdc()only for API compatibility, but it becomes a stub (returns0) with a note to useanalogReadMilliVolts()in Core 3.x. [2]
Sources: PR #21 conversation/summary [1], commit diff f541b8d [2].
Personal fork URL should not be merged to master.
The submodule points to a personal fork (dexif/community-sdk.git) instead of the official repository (open-x4-epaper/community-sdk.git). This creates a fragile dependency—if the fork is deleted, made private, or the branch is removed, all clones of this repository will fail to initialize the submodule.
The related SDK PR #21 exists and contains Arduino Core 3.x compatibility changes for the BatteryMonitor library. Update the submodule URL back to https://github.com/open-x4-epaper/community-sdk.git and ensure this branch reference is updated accordingly once the changes are available in the official repository.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitmodules around lines 3 - 4, Replace the personal fork URL and temporary
branch in the .gitmodules entry: change the url value from
"https://github.com/dexif/community-sdk.git" to
"https://github.com/open-x4-epaper/community-sdk.git" and, if/when the upstream
PR is merged, update the branch value (currently "feat/arduino-core-3x-compat")
to the official branch or tag provided by the upstream repo; ensure the
.gitmodules url and branch lines are updated together so submodule init/update
uses the official upstream source.
There was a problem hiding this comment.
Yes. After merging a pull request, need to switch to the official repository.
There was a problem hiding this comment.
@dexif, understood! That makes perfect sense. Using the temporary fork during development is a practical approach while waiting for the upstream PR to be merged.
Once open-x4-epaper/community-sdk#21 is merged to the official repository, you can update .gitmodules to point back to https://github.com/open-x4-epaper/community-sdk.git and reference the appropriate branch or tag from there. This will ensure long-term stability of the submodule dependency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/network/WebDAVHandler.cpp (2)
512-514: Destination removal may fail for directories.
Storage.remove()typically only works for files. If the destination is a directory (empty or non-empty), removal will fail silently and the subsequent rename will also fail, returning a 500 error with "Move failed" rather than a more specific error.Consider checking if destination is a directory and handling accordingly (e.g., return 409 Conflict or use recursive deletion).
♻️ Proposed enhancement
if (dstExists) { + FsFile dst = Storage.open(dstPath.c_str()); + if (dst && dst.isDirectory()) { + dst.close(); + s.send(409, "text/plain", "Cannot overwrite directory"); + return; + } + if (dst) dst.close(); Storage.remove(dstPath.c_str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/network/WebDAVHandler.cpp` around lines 512 - 514, The current dst removal uses Storage.remove(dstPath.c_str()) which only handles files and can fail silently for directories; change the dstExists handling to first detect whether dstPath is a directory (e.g., via Storage::isDirectory(dstPath) or Storage::stat/metadata on dstPath), and if it is a directory either return a 409 Conflict to the client or call a safe recursive delete routine (e.g., Storage::removeRecursive(dstPath) or a directory-specific removal method), ensuring you check and propagate any error result (log it and return an appropriate HTTP error instead of the generic "Move failed") when rename/move would otherwise proceed; keep the filename references dstPath, dstExists, and Storage.remove/removeRecursive in the updated code.
592-594: Same directory destination concern as handleMove.This has the same issue where overwriting a directory destination will fail silently. Consider applying the same fix suggested for
handleMove().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/network/WebDAVHandler.cpp` around lines 592 - 594, The current overwrite branch that calls Storage.remove(dstPath.c_str()) will silently fail for directory targets; update this code (the block that uses dstExists, dstPath and Storage.remove) to mirror the handleMove() fix: detect if dstPath is a directory (e.g., Storage.isDirectory(dstPath) or equivalent) and, if so, remove it recursively with the same directory-removal routine used in handleMove (or return a proper error if recursive delete is not allowed); otherwise call Storage.remove for files. Ensure you reference and reuse the same helper or logic used in handleMove to handle directories safely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/network/WebDAVHandler.cppsrc/network/WebDAVHandler.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/network/WebDAVHandler.h (1)
src/network/WebDAVHandler.cpp (18)
getRequestPath(660-677)getRequestPath(660-660)getDestinationPath(679-708)getDestinationPath(679-679)urlEncodePath(710-735)urlEncodePath(710-710)isProtectedPath(737-761)isProtectedPath(737-737)getDepth(763-769)getDepth(763-763)getOverwrite(771-775)getOverwrite(771-771)clearEpubCacheIfNeeded(777-782)clearEpubCacheIfNeeded(777-777)sendPropEntry(242-273)sendPropEntry(242-243)getMimeType(784-802)getMimeType(784-784)
src/network/WebDAVHandler.cpp (1)
lib/Epub/Epub.h (1)
Epub(15-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (7)
src/network/WebDAVHandler.h (1)
1-44: LGTM!The header is well-structured with a clear separation between the public RequestHandler interface and private implementation details. The PUT streaming state members and utility method declarations are appropriately organized.
src/network/WebDAVHandler.cpp (6)
11-19: LGTM!Good use of anonymous namespace for internal constants. The FIXED_DATE fallback is a reasonable approach given ESP32's lack of persistent RTC.
23-47: LGTM!The method routing correctly handles all WebDAV Class 1 methods and enables raw streaming only for PUT operations.
49-118: LGTM on temp file approach!The safe PUT implementation using a temporary file correctly addresses the previous data loss concern. The RAW_END branch properly performs atomic replacement only on success, and RAW_ABORTED cleans up appropriately.
603-615: LGTM!The streaming copy with watchdog reset and proper error checking is well-implemented. The 4KB stack buffer is reasonable for ESP32.
630-656: LGTM!The dummy LOCK/UNLOCK implementations provide adequate client compatibility for Class 1 compliance without implementing actual locking semantics.
660-802: LGTM!The utility functions are well-implemented:
- Path normalization correctly handles URL decoding and edge cases
isProtectedPathproperly checks all path segments, not just the leaf- MIME type lookup covers common file types used in e-reader context
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/network/WebDAVHandler.cpp`:
- Around line 358-361: The failure branch currently calls
Storage.remove(path.c_str()) when _putOk is false, which deletes the original
file; instead construct the temp filename (e.g. tempPath = path + ".davtmp") and
call Storage.remove(tempPath.c_str()) so only the temporary upload is removed;
keep the s.send(500, ...) and return behavior unchanged and update the code
around the _putOk check in WebDAVHandler (where Storage.remove is invoked) to
use the temp path.
---
Nitpick comments:
In `@src/network/WebDAVHandler.cpp`:
- Around line 512-514: The current dst removal uses
Storage.remove(dstPath.c_str()) which only handles files and can fail silently
for directories; change the dstExists handling to first detect whether dstPath
is a directory (e.g., via Storage::isDirectory(dstPath) or
Storage::stat/metadata on dstPath), and if it is a directory either return a 409
Conflict to the client or call a safe recursive delete routine (e.g.,
Storage::removeRecursive(dstPath) or a directory-specific removal method),
ensuring you check and propagate any error result (log it and return an
appropriate HTTP error instead of the generic "Move failed") when rename/move
would otherwise proceed; keep the filename references dstPath, dstExists, and
Storage.remove/removeRecursive in the updated code.
- Around line 592-594: The current overwrite branch that calls
Storage.remove(dstPath.c_str()) will silently fail for directory targets; update
this code (the block that uses dstExists, dstPath and Storage.remove) to mirror
the handleMove() fix: detect if dstPath is a directory (e.g.,
Storage.isDirectory(dstPath) or equivalent) and, if so, remove it recursively
with the same directory-removal routine used in handleMove (or return a proper
error if recursive delete is not allowed); otherwise call Storage.remove for
files. Ensure you reference and reuse the same helper or logic used in
handleMove to handle directories safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/network/WebDAVHandler.cpp (1)
629-641: Consider addingyield()in the copy loop for large files.The PROPFIND directory iteration uses both
yield()andesp_task_wdt_reset(), but the copy loop only uses the WDT reset. For very large files, addingyield()would improve responsiveness of other tasks.♻️ Suggested improvement
while (srcFile.available()) { esp_task_wdt_reset(); + yield(); int bytesRead = srcFile.read(buf, sizeof(buf));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/network/WebDAVHandler.cpp` around lines 629 - 641, The copy loop in WebDAVHandler.cpp that streams bytes from srcFile to dstFile (the while (srcFile.available()) { ... } block using buf[4096]) should call yield() inside the loop to improve scheduler responsiveness for large files; modify that loop to invoke yield() (for example immediately after esp_task_wdt_reset() or after each successful write) while preserving the existing esp_task_wdt_reset() and error handling so copyOk remains correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/network/CrossPointWebServer.hsrc/network/HttpDownloader.cppsrc/network/WebDAVHandler.cppsrc/network/WebDAVHandler.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/network/CrossPointWebServer.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/network/WebDAVHandler.h (1)
src/network/WebDAVHandler.cpp (18)
getRequestPath(686-703)getRequestPath(686-686)getDestinationPath(705-734)getDestinationPath(705-705)urlEncodePath(736-761)urlEncodePath(736-736)isProtectedPath(763-787)isProtectedPath(763-763)getDepth(789-795)getDepth(789-789)getOverwrite(797-801)getOverwrite(797-797)clearEpubCacheIfNeeded(803-808)clearEpubCacheIfNeeded(803-803)sendPropEntry(267-298)sendPropEntry(267-268)getMimeType(810-828)getMimeType(810-810)
src/network/HttpDownloader.cpp (2)
lib/KOReaderSync/KOReaderSyncClient.cpp (2)
isHttpsUrl(28-28)isHttpsUrl(28-28)src/util/UrlUtils.cpp (2)
isHttpsUrl(5-5)isHttpsUrl(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (17)
src/network/WebDAVHandler.h (1)
1-44: LGTM!The header is well-organized with clear separation between the public
RequestHandlerinterface and private implementation details. The PUT streaming state encapsulation (_putFile,_putPath,_putOk,_putExisted) properly supports the chunked upload flow described in the implementation.src/network/WebDAVHandler.cpp (13)
11-19: LGTM!Good use of anonymous namespace for implementation-private constants. The
FIXED_DATErationale is well-documented for ESP32 environments without RTC.
23-47: LGTM!The
canHandlecorrectly accepts all WebDAV methods, andcanRawproperly enables raw streaming only for PUT requests.
96-110: Minor atomicity gap in overwrite scenario.When
_putExistedis true, the original file is removed (line 100) before the rename attempt (line 103). If the rename fails, the original is already gone. This is a known limitation on filesystems without atomic rename-over support (like FAT32 on SD cards), and the current approach is reasonable given the constraints.The temp-file approach correctly addresses the primary data-loss concern from interrupted uploads.
120-159: LGTM!Clean dispatch pattern that correctly mirrors the methods accepted by
canHandle().
163-171: LGTM!Correct WebDAV Class 1 OPTIONS response with appropriate
DAV: 1,Allow, andMS-Author-Viaheaders for broad client compatibility.
175-265: LGTM!Solid PROPFIND implementation with proper 207 Multi-Status responses, hidden item filtering, and cooperative multitasking via
yield()andesp_task_wdt_reset()in the directory iteration loop.
302-335: LGTM!Efficient file streaming via
NetworkClient.write(file)for GET requests with proper access control and error handling.
339-369: LGTM!Correct HEAD implementation that returns appropriate headers without a body.
373-393: LGTM!The PUT finalization correctly uses the temp file cleanup path and sends appropriate 201/204 status codes based on whether the file existed.
397-446: LGTM!Proper DELETE implementation with root protection, non-empty directory checks (409 Conflict), and epub cache cleanup.
450-486: LGTM!RFC 4918-compliant MKCOL implementation with proper body rejection (415), existence check (405), and parent directory validation (409).
656-682: LGTM!Appropriate dummy LOCK/UNLOCK implementation for Class 1 WebDAV. The dummy token satisfies clients that probe for lock support without implementing actual locking semantics.
686-828: LGTM!Well-implemented utility functions with proper URL encoding/decoding, path normalization, and security checks. The
isProtectedPathcorrectly validates all path segments to prevent traversal attacks.src/network/HttpDownloader.cpp (3)
5-6: Header switch to NetworkClient looks consistent.No issues with the include swap to the new transport abstractions.
115-117:getStreamPtr()returnsNetworkClient*in Arduino-ESP32 v3.3.7—code is correct.Verification confirms that HTTPClient::getStreamPtr() in Arduino-ESP32 v3.3.7 returns NetworkClient*, matching the code at line 116. No compatibility issue.
16-25: No action needed.NetworkClientis a concrete class in Arduino-ESP32 3.3.7 with a public default constructor and can be directly instantiated.HTTPClient::begin()acceptsNetworkClient¶meter, which matches your code pattern. The implementation is correct and will compile and run without issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/network/WebDAVHandler.cpp`:
- Around line 538-540: When overwriting an existing destination (dstExists) you
must check whether dstPath is a directory and call Storage.removeDir() for
directories instead of Storage.remove(); update the block in the function
containing dstExists (and replicate the same fix in handleCopy) to: detect
destination type (use your storage API's directory check for dstPath), call
Storage.removeDir(dstPath) when it's a directory, and call
Storage.remove(dstPath) when it's a file, preserving any existing error
handling.
---
Duplicate comments:
In `@src/network/HttpDownloader.cpp`:
- Around line 67-75: The HTTP branch in HttpDownloader (where
UrlUtils::isHttpsUrl(url) is false) simply does client.reset(new
NetworkClient()) without the same verification/configuration used elsewhere (see
fetchUrl and the NetworkClientSecure path); update the else branch in
HttpDownloader::download (or the surrounding function) to mirror fetchUrl's
handling: construct or obtain the NetworkClient via the same factory or
allocation pattern, verify the pointer is non-null, perform any required
initialization or configuration checks (the same checks used for
NetworkClientSecure, e.g., connection/config validation or setInsecure if
appropriate), and log/handle errors on failure so the HTTP path is as robust as
the HTTPS path.
---
Nitpick comments:
In `@src/network/WebDAVHandler.cpp`:
- Around line 629-641: The copy loop in WebDAVHandler.cpp that streams bytes
from srcFile to dstFile (the while (srcFile.available()) { ... } block using
buf[4096]) should call yield() inside the loop to improve scheduler
responsiveness for large files; modify that loop to invoke yield() (for example
immediately after esp_task_wdt_reset() or after each successful write) while
preserving the existing esp_task_wdt_reset() and error handling so copyOk
remains correct.
| if (dstExists) { | ||
| Storage.remove(dstPath.c_str()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Storage.remove implementation
rg -n 'Storage\s*[.:]\s*remove' --type cpp -B 2 -A 10Repository: crosspoint-reader/crosspoint-reader
Length of output: 33993
🏁 Script executed:
# Also search for Storage class definition
rg -n 'class Storage' --type cpp -B 2 -A 5Repository: crosspoint-reader/crosspoint-reader
Length of output: 61
🏁 Script executed:
# Read the full handleMove function to understand the context
cat -n src/network/WebDAVHandler.cpp | sed -n '490,557p'Repository: crosspoint-reader/crosspoint-reader
Length of output: 2239
🏁 Script executed:
# Find Storage class definition
fd -e h -e hpp | xargs rg -l 'class Storage' --type cppRepository: crosspoint-reader/crosspoint-reader
Length of output: 63
🏁 Script executed:
# Search for Storage.h header
find . -name "Storage.h" -o -name "storage.h"Repository: crosspoint-reader/crosspoint-reader
Length of output: 61
🏁 Script executed:
# Look for Storage API definition - search in likely header locations
rg -n 'bool remove\(' --type cpp --type h -A 3 | head -50Repository: crosspoint-reader/crosspoint-reader
Length of output: 320
🏁 Script executed:
# Read HalStorage.h to see full API
cat -n lib/hal/HalStorage.h | head -60Repository: crosspoint-reader/crosspoint-reader
Length of output: 2543
🏁 Script executed:
# Also check for isDir or similar methods
rg -n 'isDir|isFile' lib/hal/ --type hRepository: crosspoint-reader/crosspoint-reader
Length of output: 61
Handle directory destinations properly when overwriting.
When dstPath points to an existing directory and overwrite=true, Storage.remove() will fail since it only works on files. Use Storage.removeDir() for directories or add logic to check the destination type before removal. This same issue exists in handleCopy() at line 619.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/network/WebDAVHandler.cpp` around lines 538 - 540, When overwriting an
existing destination (dstExists) you must check whether dstPath is a directory
and call Storage.removeDir() for directories instead of Storage.remove(); update
the block in the function containing dstExists (and replicate the same fix in
handleCopy) to: detect destination type (use your storage API's directory check
for dstPath), call Storage.removeDir(dstPath) when it's a directory, and call
Storage.remove(dstPath) when it's a file, preserving any existing error
handling.
|
|
|
I had a similar warning for littlefs when I cancelled the build the first time. Ended up just blowing away the |
@osteotek Because now pioarduino is used. The Platform pioarduino espressif32 has nearly nothing to do with the old Platformio espressif32 v6.12 |
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
|
I think it's important to note somewhere that developers need to delete Edit: will also need to update submodule: |
| // Collect WebDAV headers and register handler | ||
| const char* davHeaders[] = {"Depth", "Destination", "Overwrite", "If", "Lock-Token", "Timeout"}; | ||
| server->collectHeaders(davHeaders, 6); | ||
| server->addHandler(&davHandler); |
There was a problem hiding this comment.
I'm getting double free while working on #1016 , it seems like the server will free the handler automatically: https://github.com/espressif/arduino-esp32/blob/89bcb9c2c7abb6fc90784cef4a870e0e9ff03579/libraries/WebServer/src/WebServer.cpp#L52-L66
Not sure if anyone else get this problem?
Edit: can reproduce on master branch: open file transfer > join a network > back > it crashes
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary - Upgrade platform from espressif32 6.12.0 (Arduino Core 2.0.17) to pioarduino 55.03.37 (Arduino Core 3.3.7, ESP-IDF 5.5.2) - Add WebDAV Class 1 server (RFC 4918) - SD card can be mounted as a network drive - I also slightly fixed the SDK and also made a [pull request ](open-x4-epaper/community-sdk#21) First PR crosspoint-reader#1030 (was closed because the implementation was based on an old version of the libraries) Issue crosspoint-reader#439 --------- Co-authored-by: Dave Allie <[email protected]>
## Summary Ref: #1047 (comment) To reproduce: 1. Open file transfer 2. Join a network 3. Once it's connected, press (hold) back --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO**
|
Don't try to use webdav in Finder in macOS, Finder tries to download all files (probably for caching/previews) and that just freezes X4, with no option to quit from File Transfer screen. With Cyberduck it performs a little better, but still experience is rough |
5 participants
Summary
First PR #1030 (was closed because the implementation was based on an old version of the libraries)
Issue #439