search

flyingeinstein / Restfully

Arduino library for making Rest compliant APIs on the ESP8266 with embedded Uri arguments
MIT License
9 stars 2 forks source link

Compilation errors in Arduino 1.8.8 #2

Open ace42588 opened 5 years ago

ace42588 commented 5 years ago

I keep running into compilation issues when verifying the example sketch that don't make much sense. I didn't capture the compiler output from the first couple issues, but I'll paraphrase

Seems to me that I'm missing something or my library keeps getting corrupted somehow. Is there anything unique needed to compile the example sketch? I am using ArduinoJson 6.5 and included stdio.h just for chuckles

guru-florida commented 5 years ago

Hi @ace42588. As noted in part of the doc only the ArduinoJson 6.x is supported at the moment, I think that is what may be happening here. ArduinoJson had a significant interface change betwee v5 and 6.

Honestly, I would hold off from using this project for a few weeks. I am in the middle of some changes at the moment. Mostly, the use of .on(, GET(func), PUT(func)) was causing all sorts of C++ issues and cryptic error messages if function signatures weren't perfect.

For ease of use, I've changed to .on().GET(func).PUT(func) kind of syntax. In the new format the on() returns a node reference and then the .GET() and .PUT(), etc, are just chained function calling. The compiler no longer has to deduce arguments in a pseudo-magic way so even if you make an error in your callback types you'll get a more sensical error to point you in the right direction.

I am almost done the work. I am using this project in another Esp8266 project but I would love to have another tester. Are you interested? If so, can you provide me more details on what hardware and libraries (+versions) you are using? The core of the Restfully code isnt tied to any particular Json or network lib, but has a single file (Rest-Esp8266.h) that ties specific dependencies to the Rest core....so once I get my latest changes checked in I can start adding the compatibility including both ArduinoJSON APIs (<v5 and v6 onward) and non-8266 devices.

ace42588 commented 5 years ago

I'd be happy to help test.

brunokc commented 5 years ago

I'm available to test as well on PlatformIO (using Arduino framework) on a ESP32 -- I know you said you're not interested at this point, but I am :) I'd like to see if I can make it work there. Right now, I'm applying all sorts of hacks to make it compile.

I'm on:

Thanks.

brunokc commented 5 years ago

What do you know, I got it working on ESP32. Not a lot of changes needed, mostly did this:

Based on what I see here, you seem to have fixed some of these already and went beyond that, so I don't think it's worth posting a PR. Let me know if you think otherwise though.

Looking forward to another release (and push to PlatformIO).

guru-florida commented 5 years ago

@brunokc Yes, paged-pool is unused at the moment but possibly used in the future. I would like to be able to see the diff on the changes you did, are you working from a fork? Glad to see it wasnt hard to get working on ESP32!

guru-florida commented 5 years ago

Good news guys! I've completed my changes and added some great features. The hard part is over, I just have to update the Arduino examples and I can commit. (I estimate a few days max)

Breaking Changes

Joining Endpoint collections

As mentioned in features, we can join two Endpoints collections using the .with(args) method. The with method will return the right-hand Endpoints collection so you can keep using .on(), .GET(), .PUT(), etc in a method invoke chain. I am using Restfully in another project (Nimble) where the Rest interface is key to interfacing with the internal sensor devices, etc. in a plug-and-play way so splitting the Rest endpoints into multiple units was very important.

Typically, you would use with() something like this: Endpoints ep1, ep2; // this type is actually system dependent

ep1.on("/api/sys").with(ep2).on("status").GET(status_func);

or nicely indented to show endpoint level:

ep1
    .on("/api/sys")
    .with(ep2)
        .on("status")
            .GET(status_func)
        .on("statistics")
            .GET(statistics_func)

In the above example the status and statistics endpoints are actually defined in the ep2 collection, then attached to the /api/sys node of the ep1 collection. The with() method always returns the right-hand Endpoints collection.

In the above examples both collections have the same handler type (static functions). We can also change the code to use method instances for ep2:

class Sys {  status(); statistics() }
Sys mysys;

Endpoints ep1;     // this type is actually system dependent
Endpoints<Sys> ep2;
ep1
    .on("/api/sys")
    .with(mysys, ep2)
        .on("status")
            .GET(&Sys::status)
        .on("statistics")
            .GET(&Sys::statistics)

Actually, ep2 is optional here since we never actually use ep2 outside the chain. The Endpoints class can construct it as an anonymous type and store it within the parent collection.

class Sys {  status(); statistics() }
Sys mysys;

Endpoints ep1;     // this type is actually system dependent
ep1
    .on("/api/sys")
    .with(mysys)      // this creates a new anonymous collection with Sys class and returns it
        .on("status")
            .GET(&Sys::status)
        .on("statistics")
            .GET(&Sys::statistics)

The with(...) method has overloads that support these args:

Class Method Handlers vs Static Handlers

In all cases, when you use with(...) to attach an object instance to a member function the this object is bound to the member function at Request time to produce a statically invokable function call (think std::bind). So in the above examples resolving from the parent (containing static handlers) will always return a static function handler regardless of joined instance member based Endpoints.

You can also create stand-alone Endpoints that return class-method handlers as well but all method handlers in an Endpoints collection must be from the same class type. You would resolve a URL to get a class-method handler, and would perform the invoke yourself using the .* operator.

brunokc commented 5 years ago

@brunokc Yes, paged-pool is unused at the moment but possibly used in the future. I would like to be able to see the diff on the changes you did, are you working from a fork? Glad to see it wasnt hard to get working on ESP32!

I don't have a fork, I just cloned, checked out the 0.2.0-beta release and applied my changes on top on a local topic branch. I can post a PR from that branch, if you'd like. I can do that tonight when I get home.

guru-florida commented 5 years ago

@brunokc I've added you as a collaborator, so you can create a branch and post changes there. Thanks!

brunokc commented 5 years ago

@guru-florida , this update looks awesome! Looking forward to giving it a try.

guru-florida commented 5 years ago

I've checked in the changes to master.

@ace42588 Can you take a crack at it? Are you running from a Git clone or straight from Arduino lib manager (if so, then I have to do a release first and wait a day for Arduino to catch the new release)

@brunokc I've taken in your edits manually (without merging PR since the code has changed so much). The library should work with with both Esp8266 and Esp32 now. There are two files in src/Platforms/Esp8266.h and src/Platforms/Esp32.h that define the right classes and I defined the Esp32.h based on your PR diffs. I cant test it here but I bet you can figure out any mistake I made if I did :) . It should look pretty simple I think. Let me know one way or the other.

Example SimpleRestServer is updated as well.

guru-florida commented 5 years ago

I've updated the main README to document required changes and new features

brunokc commented 5 years ago

Thanks for the update, @guru-florida. Here's a summary of the things I'm seeing:

1) For some reason PlatformIO likes to compile everything under a folder (I just started using it for this project, so I may be missing something). Because of this I'm hitting some of the same problems I had to fix for 0.2.0-beta. Namely, I had to rename or remove files that didn't apply to my case (e.g.: import/paged-pool.*) so things would compile. 2) As before, I can't find memory.h, switched to string.h. Not sure how to solve this one in a general way. 3) Platforms/generics.h still had a reference to ESP8266WebServer.h. I removed it. 4) Platforms/generics.h seems to be platform agnostic. Does it really belong under the Platforms folder? 5) I have to admit having to use arrows now on RestRequestHandler (e.g.: restHandler->on(...)) threw me off, knowing that I didn't allocate it on the heap, but as a global. Is that the intention going forward? If so, I believe README.md needs to be updated to reflect that. 6) I was getting some compilation warnings due to member initialization ordering in Rest::Generics::Request's first constructor (generics.h). By moving the initialization of server to happen after the invocation of the base class' constructor (TUriRequestFragment), the warning went away.

My code is building now. I'll run some basic tests and let you know.

Thanks!

guru-florida commented 5 years ago

@brunokc I've found an ESP32 (Wemo+LCD) so I am going through updates to support both ESP8266 and ESP32. Your list is really helping!! I installed PlatformIO and gave it a shot but that's all new to me so ran into walls....I will definitely try again as the VSCode and PlatformIO plugin looks like a great pair. (1) I did remove the "import" folder with the paged and pool files. (2) I havent run into this yet on both Windows and Mac builds. Very strange. (3) Fixed. (4) I think so. The generics there are to assist in building platform configs. They tie a WebServer, a Json library and the Restfully core into a single (more friendly) solution. Those generics are not required for the Restfully core and you could use the Restfully core without it...like I do on Linux/Mac gcc/clang builds outside Arduino. (5) I agree with you actually. I did this so I didnt have to create a bunch of inline functions that just delegate to the Endpoints class. Overriding the -> just allowed a simple way to go straight to the Endpoints class. There are only 2 or 3 methods though so I should just do this and go back to dotted. Arduino guidelines say -> is not Arduino friendly. You got me on the README not updating -> usage. hehe (6) I get these warnings too, a few signed/unsiged and initialization order. I will post a fix soon.

I updated the Restfully example to support ESP32 with minimal #define tests (i.e. 8266 or ESP32?). I am currently debugging why on ESP32 the LED endpoints arent working though, returning 404.

guru-florida commented 5 years ago

Ok, fixed the 404 issue. I have the example working well on both Esp8266 and ESP32 devices!

(5) Also, you dont need to use the -> operator anymore. On closer look it is only the on(...) method needed on the RestHandler class and from the first on() all the other methods are accessible on the Node class. So a single inline forwarding method did the trick.

(2) I still havent run into the memory.h / string.h issue. Can you tell me exactly where that occured?

(6) I fixed the initialization order. This is an error on ESP32 and warning on 8266.

Can you try out the latest asap? I would like to generate a release so Arduino Library Manager picks up the new version....the old version is pretty bad at this point. :) I am concerned about the memory/string.h though.

brunokc commented 5 years ago

Cool, I believe I ran into the 404 issue last night as well but ran out of time to investigate. I'll try these new changes as soon as I get home later tonight. Thanks!

brunokc commented 5 years ago

Still not working for me. Getting 404 on "/api/v1.0/wifi/foo" using GET(). I'm setting it up as:

restHandler.on("/api/v1.0/wifi/foo").GET(HandleFoo);

I can't debug because, well, because I don't know how to yet. :) Besides, it looks like I'd need to pay in order to get debugging support from PlatformIO. I'll eventually need debugging, but haven't had the chance to look into yet just yet. However, I went as far as adding some println's and I found out that I'm failing here:

ArduinoPlatform.h@85 in WebServerRequestHandler<>::handle(...):

...
                typename Endpoints::Request ep = endpoints.resolve(method, requestUri.c_str());
                Serial.println("Restfully: method=" + String{ method } + "; uri=" + requestUri + "; ep:" + (bool)ep);
                if (ep) {
...

ep is resolving to false for some reason in the if (ep) line. I haven't changed my original code for that particular URI, and it was working before, so I don't think I'm doing anything wrong. Any ideas?

Another note: I don't see code anywhere initializing request.httpStatus. Yet, we're comparing it to 0 a few lines below, after the call to ep.handler(...):

                    int rs = ep.handler(request);
                    if (request.httpStatus == 0) {
                        if (rs == 0)
                            request.httpStatus = 200;
                        else if (rs < 200)
                            request.httpStatus = 400;
                        else
                            request.httpStatus = rs;
                    }

I think the outer if can be dropped.

As far as the memory.h problem is concerned, it may have something to do with my environment (PlatformIO). Basically, the #include <memory.h> in pool.cc was failing for me with 'file not found'. Removing it didn't work because of the dependency on memset(). I looked around and found memset() declared in string.h, so I replaced memory.h with string.h and I was back in business. Having said all that, it looks like you just solved the problem altogether by removing pool.cc from the build, so I think we're good on that front.

Thanks!

edit: ep can't be nullptr, so clarified that ep is resolving to false in a boolean context as in the if (ep) line.

guru-florida commented 5 years ago

@ace42588 To test, can you remove the period i "v1.0"? maybe I have a scanning issue with not recognizing the period character. I can try to reproduce your issue here.

memory.h issue: Ah, this makes sense now. The first thing I did was remove the import folder thus why I didnt get this issue. :)

request.httpStatus is initialized to 0 in the constructor (generics.h, line 72). It is accessible from the handler, so the handler or an error function can set HTTP status. Seems redundant and I may remove httpStatus in favor of just the handler return value. In another project it was useful to be able to set httpStatus inside deeper function calls. I had "Input Policy" validation functions. These input policy's expected a parameter to exist (or optional) and would perform validation and if it passed validation would return the C structure, collection or element that represented the validated input. Each API call then had a supported list of input validators. It made sure that all inputs to each API call were consistent and validated and errors were also consistent. A validation policy function could cause an API call to fail and report an appropriate error message and http status (things like InvalidArgument, MissingArgument, BadRequest, ExpectedInteger/String/etc.) Many API call implementations were also shorter because the input validator returned the actual C pointer/ref/array to be operated on....We have 190 API calls in that API and this definitely worked out well. At some point I would like to add support for building these input validators into Restfully.

brunokc commented 5 years ago

Had to leave to work in a hurry this morning. I'll try it once I get home.

Duh on httpStatus. Totally missed the initialization. Sorry for the false alarm. Totally understand the motivation for input policies. Didn't mean to question it, was just wondering if my mistaken take on httpStatus not being initialized was causing the issue I was seeing.

Let me know if you get to try anything with a dot as part of the URI (as in "v1.0" in my example). Otherwise, I'll give it a go once I get home.

guru-florida commented 5 years ago

It's true, my URL tokenizer didnt like the dot. I just committed a checkin to add the dot to that char class. When adding the expressions in the on(uri) statement the paths there are expected to be like 'identifiers'. They support alpha-nums and -_ and now dots. For arguments in the resolve() step pretty much anything goes (URI encoding rules). I will have to rethink if I should be restricting that or not. It's not that I have to, was just what I wanted for my other project. Thoughts?

FYI Tokenizer is in Token.h and it should be pretty easy to see how it works. It is a pretty typical "scanner" class if you've ever taken compiler design. Scanner has slight differences depending on if it is being used in the on() clause or resolve()ing a real URL (allow_parameters=0 or 1)

brunokc commented 5 years ago

Confirmed the fix works for me. Now I'm back to where I was with version 0.2.0-beta.

To answer your question, I see many URIs containing a version, sometimes with a dot, sometimes without (e.g.: v1, v2). Although you could decide to restrict it, I would imagine it would have to be a deliberate decision as dots are accepted in URIs and you're taking URIs as argument. Not saying it's wrong to block it, just saying I don't see a lot of reasons to do it. In any case, this is one of those decisions that, as of right now, is easy to change later. In that spirit, I'd pick the one that makes the most sense to you and move on. One day, if it becomes a problem, you can address it.

Thanks for the pointers on Token.h. I'm still not that familiar with the code and not having a debugger available limits my understanding even more. I just found out that OpenOCD is compatible with the ESP32 and I'm trying to settle on a JTAG adapter to use with it. Once I have it, I should be able to use it with VisualGDB or perhaps even VSCode as a frontend for GDB. At that point I should be able to navigate the code much easier. How are you debugging your code?

Thanks!

brunokc commented 5 years ago

On, forgot to mention. Would it be possible to add an example of the usage of the POST method with some JSON in the request body and in the response body? So far I haven't had any luck getting Restfully to recognize the JSON payload in the request body. Apparently, trying to access request["foo"] (while having "foo" as part of the JSON in the request body) causes a crash.

Note that I don't have any tokens in my URI (e.g.: "/api/v1.0/container"). It looks like using operator[] on request is supposed to be used to access parameters in the URI (e.g.: "pin" or "value" in your PUT example). However, I want to access properties of the request body. How do I do that?

Thanks!

guru-florida commented 5 years ago

My bad. I forgot to wire up the parsing of POST data into the request doc. It works now. See the echo API call in the SimpleRestServer example. Example:

if(request.hasJson) {
    auto root = request.body.as<JsonObject>();  // expect request is a Json object not array
    auto greeting = root.get<const char*>("greeting");  // NULL if this argument doesnt exist
    if(greeting) {
      // do something with parameter
    }
}

The json is only parsed if content-type="application/json". Dont add any "; charset..." either, I will have to allow this later.

You can still access query parameters via the request.server as normal WebServer.on() requests would. But I inline redirected request.queryArg() to the arg() functions in server.

Most of my development of the library has been in CLion IDE on Mac and Windows using cmake to build. CLion works great as an IDE with debugging, intellisense, code refactoring features. Makes development much faster than Arduino compile/upload cycles. This is also where the unit tests are.

brunokc commented 5 years ago

Thanks for fixing that. I'll check it out later tonight. Thanks for that pointer to CLion. I hadn't heard of it. I'll check it out.

guru-florida commented 5 years ago

FYI I now have two ESP32s (WeMos LOLA and HelTec WifiKit32). I've noticed a very significant difference in WebServer response between the ESP32 and Esp8266. Talking less than a second compared to upwards of 5 seconds. I have no idea why it would take any noticeable time to request from a device on the same Wifi. Seems like I am not the only one: https://forum.arduino.cc/index.php?topic=532207.0

Have you had similar experience?

brunokc commented 5 years ago

I only have a single ESP32 board (I haven't used ESP8266 yet) so I don't really have a baseline. Having said that, I'd be happy to try a few things. If you have a piece of code you'd like me to run on mine to measure something, I'd be happy to try.

I currently have my ESP32 initially configured as an AP, serving out 1 HTML file, 3 or 4 CSS files (including big ones like Bootstrap), 3 JS files (including JQuery) e a few images. Each file is served out of the SPIFFS partition of the flash and it's sent pretty much instantly. The whole page loads in a browser connected over WiFi directly to the ESP32 in about 2-3 seconds.

guru-florida commented 5 years ago

And I assume you are using the standard WebServer class, not the Async one or a different project? Even the WebServer example for my Heltec WifiKit 32 is taking forever. It ranges, but up to 45 seconds is common!! This is just the simple HelloWorld example included with the board. Take a look at the screenshot.

HelloWorld45s
guru-florida commented 5 years ago

Ok, worst is the Heltec. I plugged the WeMos back in and it averages about 340ms but rarely takes more than a second. I repeated tests with the 8266 though and its worse case is <80ms and averages about 35ms...so it's easily 10 times faster than the WeMos. I did the tests with both the WebServer example and with my RestWebServer and results were the same. So the Rest handler doesnt add any delay.

Strange esp8266 is the winner considering its an older and apparently slower processor. I guess I will stick with the Esp8266 for now.