Calibre Web Epub Downloading + Calibre Wireless Device Syncing#219
Conversation
Also made the menu items dynamic and capital case. Looks better from a design perspective but I suppose this is an optional thing to accept
|
Oh and this can be tested if anyone wants to by putting https://home.jmitch.com/books as the calibre web url |
|
OPDS is a huge add, thanks for this @itsthisjustin, given the size of the PR, I'll need to take a little bit to review this, but excited for it! |
|
Totally understand. Also added Calibre Wireless Device syncing to this as well so you can auto discover Calibre servers and send to device from there. Working on koreader sync in another branch now |
src/CrossPointSettings.cpp
Outdated
| int CrossPointSettings::getReaderSideMargin() const { | ||
| switch (sideMargin) { | ||
| case MARGIN_NONE: | ||
| return 0; | ||
| case MARGIN_SMALL: | ||
| default: | ||
| return 5; | ||
| case MARGIN_MEDIUM: | ||
| return 20; | ||
| case MARGIN_LARGE: | ||
| return 30; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would love to keep this split out to avoid merge conflicts with other work, and to not over inflate large PRs
| // Prepend http:// if no protocol specified (server will redirect to https if needed) | ||
| std::string ensureProtocol(const std::string& url) { | ||
| if (url.find("://") == std::string::npos) { | ||
| return "http://" + url; | ||
| } | ||
| return url; | ||
| } | ||
|
|
||
| // Extract host with protocol from URL (e.g., "http://example.com" from "http://example.com/path") | ||
| std::string extractHost(const std::string& url) { | ||
| const size_t protocolEnd = url.find("://"); | ||
| if (protocolEnd == std::string::npos) { | ||
| // No protocol, find first slash | ||
| const size_t firstSlash = url.find('/'); | ||
| return firstSlash == std::string::npos ? url : url.substr(0, firstSlash); | ||
| } | ||
| // Find the first slash after the protocol | ||
| const size_t hostStart = protocolEnd + 3; | ||
| const size_t pathStart = url.find('/', hostStart); | ||
| return pathStart == std::string::npos ? url : url.substr(0, pathStart); | ||
| } | ||
|
|
||
| // Build full URL from server URL and path | ||
| // If path starts with /, it's an absolute path from the host root | ||
| // Otherwise, it's relative to the server URL | ||
| std::string buildUrl(const std::string& serverUrl, const std::string& path) { | ||
| const std::string urlWithProtocol = ensureProtocol(serverUrl); | ||
| if (path.empty()) { | ||
| return urlWithProtocol; | ||
| } | ||
| if (path[0] == '/') { | ||
| // Absolute path - use just the host | ||
| return extractHost(urlWithProtocol) + path; | ||
| } | ||
| // Relative path - append to server URL | ||
| if (urlWithProtocol.back() == '/') { | ||
| return urlWithProtocol + path; | ||
| } | ||
| return urlWithProtocol + "/" + path; | ||
| } |
There was a problem hiding this comment.
These have utility outside of this activity, and avoid bloating this activity lets move them into their own UrlUtils (or something similarly named) class
| std::string downloadUrl = buildUrl(SETTINGS.opdsServerUrl, book.href); | ||
|
|
||
| // Create sanitized filename | ||
| std::string filename = "/" + sanitizeFilename(book.title) + ".epub"; |
There was a problem hiding this comment.
Could we please make the downloaded file name be <title> - <author>.epub
src/activities/home/HomeActivity.cpp
Outdated
| std::vector<const char*> menuItems; | ||
| menuItems.push_back("Browse Files"); | ||
| if (hasBrowserUrl) { | ||
| menuItems.push_back("Calibre Library"); | ||
| } | ||
| menuItems.push_back("File Transfer"); | ||
| menuItems.push_back("Settings"); |
There was a problem hiding this comment.
nit: slightly cleaner way here might be instantiate the vector with the fixed three options and insert the fourth
std::vector<const char*> menuItems = { "Browse Files", "File Transfer", "Settings" };
if (hasBrowserUrl) {
// Insert calibre library to be after browse files
menuItems.insert(menuItems.begin() + 1, "Calibre Library");
}
src/activities/home/HomeActivity.cpp
Outdated
| int idx = 0; | ||
| const int continueIdx = hasContinueReading ? idx++ : -1; | ||
| const int browseFilesIdx = idx++; | ||
| const int browseBookIdx = hasBrowserUrl ? idx++ : -1; |
There was a problem hiding this comment.
nit: can you please update these variables to be more reflective of their relationship to the OPDS browser, they are a bit confusing at the moment: hasOdpsUrl, browseOdpsFilesIdx, onOdpsBrowserOpen
src/main.cpp
Outdated
| if (strlen(SETTINGS.opdsServerUrl) == 0) { | ||
| enterNewActivity(new KeyboardEntryActivity( | ||
| renderer, mappedInputManager, "Calibre Web URL", "", 10, 127, false, | ||
| [](const std::string& url) { | ||
| strncpy(SETTINGS.opdsServerUrl, url.c_str(), sizeof(SETTINGS.opdsServerUrl) - 1); | ||
| SETTINGS.opdsServerUrl[sizeof(SETTINGS.opdsServerUrl) - 1] = '\0'; | ||
| SETTINGS.saveToFile(); | ||
| exitActivity(); | ||
| enterNewActivity(new OpdsBookBrowserActivity(renderer, mappedInputManager, onGoHome)); | ||
| }, | ||
| [] { | ||
| exitActivity(); | ||
| onGoHome(); | ||
| })); | ||
| } else { |
There was a problem hiding this comment.
I don't think it's possible for this branch to ever be hit as HomeActivity validates that the value is set before calling this. It would simplify this file to remove it. Additionally, OpdsBookBrowserActivity already gracefully handles a missing SETTINGS.opdsServerUrl, so there's no risk of a crash or anything.
src/main.cpp
Outdated
| // Check WiFi connectivity first | ||
| if (WiFi.status() != WL_CONNECTED) { | ||
| enterNewActivity(new WifiSelectionActivity(renderer, mappedInputManager, [](bool connected) { | ||
| exitActivity(); | ||
| if (connected) { | ||
| launchBrowserWithUrlCheck(); | ||
| } else { | ||
| onGoHome(); | ||
| } | ||
| })); | ||
| } else { | ||
| launchBrowserWithUrlCheck(); | ||
| } |
There was a problem hiding this comment.
Is it even possible for the wifi to be connected before this function is called?
There was a problem hiding this comment.
Additionally, WifiSelectionActivity is not responsible for turning on and off the Wifi chip, even though it does a mode set when starting the scan (which by proxy turns on the wifi). Critically, the changes here never turn off the wifi when existing the OPDS brower, resulting in the Wifi being left on.
I would love all this existing logic (if necessary) and the Wifi control logic to be wrapped up into the activity (or an activity which launches the OpdsBookBrowserActivity). main.cpp should not need to know that OpdsBookBrowserActivity requires an active Wifi connection or that SETTINGS.opdsServerUrl is set, that should be a responsibility of the activity.
There was a problem hiding this comment.
Is it even possible for the wifi to be connected before this function is called?
Soooo not now? But I was thinking in the future it might. (especially now that I have KOReader Sync working, it might be a thing that the user sets the device to connect to wifi at some interval to sync on their behalf)
There was a problem hiding this comment.
It just becomes a bit messy understanding the responsibility for enabling/disabling Wifi.
Given the battery drain of using wifi, I would love for the functions using it to be wrapped within blocks enabling and disabling it.
… how the KOReader PR works too
| static constexpr uint16_t UDP_PORTS[] = {54982, 48123, 39001, 44044, 59678}; | ||
| static constexpr size_t UDP_PORT_COUNT = 5; | ||
| static constexpr uint16_t LOCAL_UDP_PORT = 8134; // Port to receive responses |
There was a problem hiding this comment.
If these are only needed as static private variables, please move them into an anonymous namespace in the CPP file. Additionally, if UDP_PORTS is statically assigned, you should just be able to use std::size(UDP_PORTS) in place of UDP_PORT_COUNT
| // Calibre protocol opcodes (from calibre/devices/smart_device_app/driver.py) | ||
| static constexpr int OP_OK = 0; | ||
| static constexpr int OP_SET_CALIBRE_DEVICE_INFO = 1; | ||
| static constexpr int OP_SET_CALIBRE_DEVICE_NAME = 2; | ||
| static constexpr int OP_GET_DEVICE_INFORMATION = 3; | ||
| static constexpr int OP_TOTAL_SPACE = 4; | ||
| static constexpr int OP_FREE_SPACE = 5; | ||
| static constexpr int OP_GET_BOOK_COUNT = 6; | ||
| static constexpr int OP_SEND_BOOKLISTS = 7; | ||
| static constexpr int OP_SEND_BOOK = 8; | ||
| static constexpr int OP_GET_INITIALIZATION_INFO = 9; | ||
| static constexpr int OP_BOOK_DONE = 11; | ||
| static constexpr int OP_NOOP = 12; // Was incorrectly 18 | ||
| static constexpr int OP_DELETE_BOOK = 13; | ||
| static constexpr int OP_GET_BOOK_FILE_SEGMENT = 14; | ||
| static constexpr int OP_GET_BOOK_METADATA = 15; | ||
| static constexpr int OP_SEND_BOOK_METADATA = 16; | ||
| static constexpr int OP_DISPLAY_MESSAGE = 17; | ||
| static constexpr int OP_CALIBRE_BUSY = 18; | ||
| static constexpr int OP_SET_LIBRARY_INFO = 19; | ||
| static constexpr int OP_ERROR = 20; |
There was a problem hiding this comment.
Would it make more sense to declare these as an enum mapping to uint8_t values?
| {"Justify", "Left", "Center", "Right"}}, | ||
| {"Reader Side Margin", SettingType::ENUM, &CrossPointSettings::sideMargin, {"None", "Small", "Medium", "Large"}}, | ||
| {"Calibre Web URL", SettingType::ACTION, nullptr, {}}, | ||
| {"Calibre Wireless Device", SettingType::TOGGLE, &CrossPointSettings::calibreWirelessEnabled, {}}, |
There was a problem hiding this comment.
Why does this need to be a TOGGLE and not an ACTION? Afaict you're always just disabling/enabling the setting when it's selected and the sub activity is entered / left. Is there any real need for globalisation or persistence of this value?
| // Report 10GB free space | ||
| sendJsonResponse(OP_OK, "{\"free_space_on_device\":10737418240}"); |
There was a problem hiding this comment.
Can you add a future TODO here so we can update it with the actual card free space in the future?
|
|
||
| // Draw progress if receiving | ||
| if (state == CalibreWirelessState::RECEIVING && currentFileSize > 0) { | ||
| const int percent = static_cast<int>((bytesReceived * 100) / currentFileSize); |
There was a problem hiding this comment.
This will overflow for books over ~20.5MB. Probably the safest way around this is:
static_cast<int>(static_cast<float>(bytesReceived) / static_cast<float>(currentFileSize) * 100f);There was a problem hiding this comment.
Any idea what the average size of an epub that people are putting on this device? Curious as I was also trying to figure out a safe number for the free space
There was a problem hiding this comment.
10GB is totally fine, just doesn't protect against a full SD card in the future. Most of my EPUBs are only a few MB, but there are some chonkers, like Wind and Truth is 23.4MB, which would overflow here.
| // Progress bar | ||
| const int barWidth = pageWidth - 100; | ||
| const int barHeight = 20; | ||
| const int barX = 50; | ||
| const int barY = statusY + 20; | ||
|
|
||
| renderer.drawRect(barX, barY, barWidth, barHeight); | ||
| renderer.fillRect(barX + 2, barY + 2, (barWidth - 4) * percent / 100, barHeight - 4); | ||
|
|
||
| // Percentage text | ||
| const std::string percentText = std::to_string(percent) + "%"; | ||
| renderer.drawCenteredText(UI_10_FONT_ID, barY + barHeight + 15, percentText.c_str()); |
There was a problem hiding this comment.
I noticed that you've done a progress bar here but, not for the OPDS downloader. GIven its reuse here, in the OTA downloader, and in the OPDS downloader, let's extract it out to a new function in ScreenComponents and allow it to take x, y, width, height, percent
There was a problem hiding this comment.
Yep good call out. I did this after the fact and it SHOULD go here as well
| std::string CalibreWirelessActivity::sanitizeFilename(const std::string& name) const { | ||
| std::string result; | ||
| result.reserve(name.size()); | ||
|
|
||
| for (char c : name) { | ||
| if (c == '/' || c == '\\' || c == ':' || c == '*' || c == '?' || c == '"' || c == '<' || c == '>' || c == '|') { | ||
| result += '_'; | ||
| } else if (c >= 32 && c < 127) { | ||
| result += c; | ||
| } | ||
| } | ||
|
|
||
| // Trim leading/trailing spaces and dots | ||
| size_t start = 0; | ||
| while (start < result.size() && (result[start] == ' ' || result[start] == '.')) { | ||
| start++; | ||
| } | ||
| size_t end = result.size(); | ||
| while (end > start && (result[end - 1] == ' ' || result[end - 1] == '.')) { | ||
| end--; | ||
| } | ||
|
|
||
| return result.substr(start, end - start); | ||
| } |
There was a problem hiding this comment.
GIven the reuse, it would be good to split this out into some common string utils
|
Side note @daveallie I found that switching between my two branches soft bricked my device as it had settings from one flash that didn't exist in the next so I'm curious if we should add a failsafe to reset the settings file if this happens so that at least the device boots |
…eb-and-margins # Conflicts: # src/CrossPointSettings.cpp # src/CrossPointSettings.h # src/activities/settings/SettingsActivity.cpp
|
I'll resist looking back through every commit/comment in the PR, please rerequest my review via GH's review system or by comment once it's good for another look |
|
|
Only thing I want to make sure you saw was that while you were reviewing I changed the Calibre settings into a submenu system to avoid an ever growing settings list. I'd recommend eventually we do this to other settings like "Reader Preferences", "Device Sleep Settings" or whatever we can group into submenus 2976113 |
|
Yeah definitely on board with this change, was mentioning it elsewhere, happy for this to be the first group to move. |
daveallie
left a comment
There was a problem hiding this comment.
Few more bits, plus some comments in CalibreWirelessActivity which need addressing/replies
src/main.cpp
Outdated
| #include "activities/browser/OpdsBookBrowserActivity.h" | ||
| #include "activities/home/HomeActivity.h" | ||
| #include "activities/network/CrossPointWebServerActivity.h" | ||
| #include "activities/network/WifiSelectionActivity.h" |
There was a problem hiding this comment.
Need Opds because it's what allows the onGoToBrowser() to work
There was a problem hiding this comment.
onGoToBrowser just calls enterNewActivity(new OpdsBookBrowserActivity(renderer, mappedInputManager, onGoHome)); which has no dependency on WifiSelectionActivity?
There was a problem hiding this comment.
I'll double check but the opds browser doesnt technically launch until the wifi selection activity shows. It's a prereq for showing the browser that they must connect to a wifi network first.
src/main.cpp
Outdated
| #include "activities/reader/ReaderActivity.h" | ||
| #include "activities/settings/SettingsActivity.h" | ||
| #include "activities/util/FullScreenMessageActivity.h" | ||
| #include "activities/util/KeyboardEntryActivity.h" |
src/main.cpp
Outdated
| #include <InputManager.h> | ||
| #include <SDCardManager.h> | ||
| #include <SPI.h> | ||
| #include <WiFi.h> |
|
Any chance you can add the option for username and password for opds. To allow access to Calibre-web Automated. |
Totally, but I'd say let's get this merged first so it's a smaller PR later to add that. I can add it this week |
If it's setup anything like my personal setup, you can use HTTP basic auth in the URL to do username and password, so instead of |
|
Would love your thoughts on these ones before merging:
Also looks like there's a small formatting issue which can be fixed by running the format script, but other than that, this looks good! |
Resolves conflicts in CrossPointSettings.cpp and SettingsActivity.cpp: - Combines opdsServerUrl (feature branch) with screenMargin and sleepScreenCoverMode (master) in settings persistence - Updates settings count to 16 persisted fields - Merges settings UI to include all new settings plus Calibre Settings action - Adopts new SettingInfo factory pattern from master
I think all of these are done no? |
I wasn't 100% sure this wasn't left over from me but gitblame says it came from someone else. Either way I don't see anywhere it's used so I THINK it's dead code
Of course. Email me. [email protected] |
The original feature assumed SSL and failed if you didn't have a cert. PR incoming |
|
Hey guys! Sorry to bother I was thinking of buying this ereader and install crosspoint, however one of the non-negotiables im looking at is progress syncing through calibre-web with both the xteink x4 and my kobo libra (it's already working through kobo devices just fine) As long as i understand, this PR has just added this feature to the device, so in theory if i install crosspoint i can configure the calibre endpoint just as i did with my kobo and sync all books progress through both of them, right? |
This isn't for syncing progress. Do you run KOReader on your kobo? |
|
@daveallie did anything change in main between this working and my merge related to networking? This 100% worked in testing and now is missing kb when a book finishes transfer. I've fixed the disconnect issue but transferring still isn't working and I can't figure out why |
…point-reader#219) Adds support for browsing and downloading books from a Calibre-web server via OPDS. How it works 1. Configure server URL in Settings → Calibre Web URL (e.g., https://myserver.com:port I use Cloudflare tunnel to make my server accessible anywhere fwiw) 2. "Calibre Library" will now show on the the home screen 3. Browse the catalog - navigate through categories like "By Newest", "By Author", "By Series", etc. 4. Download books - select a book and press Confirm to download the EPUB to your device Navigation - Up/Down - Move through entries - Confirm - Open folder or download book - Back - Go to parent catalog, or exit to home if at root - Navigation entries show with > prefix, books show title and author - Button hints update dynamically ("Open" for folders, "Download" for books) Technical details - Fetches OPDS catalog from {server_url}/opds - Parses both navigation feeds (catalog links) and acquisition feeds (downloadable books) - Maintains navigation history stack for back navigation - Handles absolute paths in OPDS links correctly (e.g., /books/opds/navcatalog/...) - Downloads EPUBs directly to the SD card root Note The server URL should be typed to include https:// if the server requires it - HTTP→HTTPS redirects may cause SSL errors on ESP32. * I also changed the home titles to use uppercase for each word and added a setting to change the size of the side margins --------- Co-authored-by: Dave Allie <[email protected]>
…point-reader#219) ## Summary Adds support for browsing and downloading books from a Calibre-web server via OPDS. How it works 1. Configure server URL in Settings → Calibre Web URL (e.g., https://myserver.com:port I use Cloudflare tunnel to make my server accessible anywhere fwiw) 2. "Calibre Library" will now show on the the home screen 3. Browse the catalog - navigate through categories like "By Newest", "By Author", "By Series", etc. 4. Download books - select a book and press Confirm to download the EPUB to your device Navigation - Up/Down - Move through entries - Confirm - Open folder or download book - Back - Go to parent catalog, or exit to home if at root - Navigation entries show with > prefix, books show title and author - Button hints update dynamically ("Open" for folders, "Download" for books) Technical details - Fetches OPDS catalog from {server_url}/opds - Parses both navigation feeds (catalog links) and acquisition feeds (downloadable books) - Maintains navigation history stack for back navigation - Handles absolute paths in OPDS links correctly (e.g., /books/opds/navcatalog/...) - Downloads EPUBs directly to the SD card root Note The server URL should be typed to include https:// if the server requires it - HTTP→HTTPS redirects may cause SSL errors on ESP32. ## Additional Context * I also changed the home titles to use uppercase for each word and added a setting to change the size of the side margins --------- Co-authored-by: Dave Allie <[email protected]>




Summary
Adds support for browsing and downloading books from a Calibre-web server via OPDS.
How it works
Navigation
Technical details
Note
The server URL should be typed to include https:// if the server requires it - HTTP→HTTPS redirects may cause SSL errors on ESP32.
Additional Context