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.

Sign up for GitHub

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

Jump to bottom

/rfid endpoint - Serve with chunked response #280

Merged
merged 8 commits into from
Dec 29, 2023
Merged

/rfid endpoint - Serve with chunked response #280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3e2b0f6
rfid chunked response
tueddy Dec 12, 2023
788d4ff
fixes
tueddy Dec 13, 2023
0d9c7b3
make clang check happy
tueddy Dec 13, 2023
b080121
endpoint /rfid returns tag details (as before)
tueddy Dec 14, 2023
86fa4f4
clang
tueddy Dec 14, 2023
871df02
fix error: inconsistent types 'int' and 'unsigned int' deduced for la…
tueddy Dec 14, 2023
403d2e5
nvsIndex is size_t
tueddy Dec 29, 2023
9c3b512
ids->ids-only
tueddy Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion html/management.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,7 +1746,7 @@ <h5 class="modal-title" data-i18n="tools.nvserase.title"></h5>

const rfidListSavedAssignments = document.getElementById("rfidListSavedAssignments");
async function rebuildRFIDList() {
var rfidList = await (await fetch("/rfid")).json();
var rfidList = await (await fetch("/rfid/details")).json();
var rfidActive = document.getElementById('rfidIdMusic').value;

rfidListSavedAssignments.innerHTML = "";
Expand Down
53 changes: 34 additions & 19 deletions src/Web.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ void webserverStart(void) {

// RFID
wServer.on("/rfid", HTTP_GET, handleGetRFIDRequest);
wServer.addRewrite(new OneParamRewrite("/rfid/details", "/rfid?details=true"));
Copy link
Contributor

Choose a reason for hiding this comment

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

OneParamRewrite is usually for path parameters in the form of /path/{some_param} -> /path?some_param={some_param} because AsyncWebServer cannot handle path params natively.

Does this even work and do something? Even if it does, using the "normal" AsyncWebRewrite makes more sense: https://github.com/me-no-dev/ESPAsyncWebServer#param-rewrite-with-matching

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it works fine, do you have a concrete improved code here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't test it, but just replacing 'OneParamRewrite' with 'AsyncWebRewrite' should do the same thing.

wServer.addHandler(new AsyncCallbackJsonWebHandler("/rfid", handlePostRFIDRequest));
wServer.addRewrite(new OneParamRewrite("/rfid/{id}", "/rfid?id={id}"));
wServer.on("/rfid", HTTP_DELETE, handleDeleteRFIDRequest);
Expand Down Expand Up @@ -1692,35 +1693,44 @@ bool DumpNvsToArrayCallback(const char *key, void *data) {
return true;
}

static String tagIdToJsonStr(const char *key) {
StaticJsonDocument<512> doc;
JsonObject entry = doc.createNestedObject(key);
if (!tagIdToJSON(key, entry)) {
return "";
static String tagIdToJsonStr(const char *key, const bool withDetails) {
if (withDetails) {
StaticJsonDocument<512> doc;
JsonObject entry = doc.createNestedObject(key);
if (!tagIdToJSON(key, entry)) {
return "";
}
String serializedJsonString;
serializeJson(entry, serializedJsonString);
return serializedJsonString;
} else {
return "\"" + String(key) + "\"";
}
String serializedJsonString;
serializeJson(entry, serializedJsonString);
return serializedJsonString;
}

uint16_t nvsIndex;

// Handles rfid-assignments requests (GET)
// /rfid returns an array of tag-id keys
// /rfid/details returns an array of tag-ids and details. Optional GET param "id" to list only a single assignment.
static void handleGetRFIDRequest(AsyncWebServerRequest *request) {

String tagId, json = "";
String tagId = "";

if (request->hasParam("id")) {
tagId = request->getParam("id")->value();
}

if ((tagId != "") && gPrefsRfid.isKey(tagId.c_str())) {
// return single RFID entry
json = tagIdToJsonStr(tagId.c_str());
String json = tagIdToJsonStr(tagId.c_str(), true);
request->send(200, "application/json", json);
return;
}
static std::vector<String> nvsKeys {};
// get tag details or just an array of key names
bool withDetails = request->hasParam("details");

std::vector<String> nvsKeys {};
static uint16_t nvsIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment on line 1747.

Suggested change
static uint16_t nvsIndex;

nvsKeys.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed since nvsKeys was just created (since it's not a static variable any more), so it's already empty.

Suggested change
nvsKeys.clear();

// Dumps all RFID-keys from NVS into key array
listNVSKeys("rfidTags", &nvsKeys, DumpNvsToArrayCallback);
Expand All @@ -1732,33 +1742,38 @@ static void handleGetRFIDRequest(AsyncWebServerRequest *request) {
// construct chunked repsonse
nvsIndex = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

see 1747

Suggested change
nvsIndex = 0;

AsyncWebServerResponse *response = request->beginChunkedResponse("application/json",
[](uint8_t *buffer, size_t maxLen, size_t index) {
[nvsKeys = std::move(nvsKeys), withDetails](uint8_t *buffer, size_t maxLen, size_t index) {
maxLen = maxLen >> 1; // some sort of bug with actual size available, reduce the len
size_t len = 0;
static String json;
String json;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move nvsIndex into the lambda here. Also nvsIndex should have the same type as std::vector::size().

Suggested change
String json;
static size_t nvsIndex = 0;
String json;


if (nvsIndex == 0) {
// start, write first tag
json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
len += sprintf(((char *) buffer), "[%s", json.c_str());
json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str(), withDetails);
if (json.length() > maxLen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You check that, but 4 lines later you're writing json.length() + 1 bytes, so you should also check for json.length() + 1 available space

Log_Println("/rfid: Buffer too small", LOGLEVEL_ERROR);
return len;
}
len += snprintf(((char *) buffer), maxLen, "[%s", json.c_str());
nvsIndex++;
}
while (nvsIndex < nvsKeys.size()) {
// write tags as long we have enough room
json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str(), withDetails);
if ((len + json.length()) > maxLen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue, check is for 1 byte too few because of the comma

break;
}
len += sprintf(((char *) buffer + len), ",%s", json.c_str());
len += snprintf(((char *) buffer + len), maxLen, ",%s", json.c_str());
nvsIndex++;
}
if (nvsIndex == nvsKeys.size()) {
// finish
len += sprintf(((char *) buffer + len), "]");
len += snprintf(((char *) buffer + len), maxLen, "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

That byte is written without a length check at all.

nvsIndex++;
}
return len;
});
nvsKeys.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw, that we are accessing nvsKeys here. Since we called std::move on the variable, nvsKeys does not hold the array any more. As soons as the lambds is finished, the nvsKeys will go out of scope and it'll be destoryed.

Suggested change
nvsKeys.clear();

request->send(response);
}

Expand Down
Loading