aster94 / GoProControl

Arduino library to interface with GoPro cameras
GNU General Public License v3.0
121 stars 21 forks source link

modifications for getmedialist / add setconnection() #36

Closed damiendon closed 3 years ago

damiendon commented 3 years ago

Hi, I made some modifications for the function getmedialist. it don't work on port 80 with gopro hero3, but on port 8080. I don't know if port 80 is working with other cameras. So i create the same functions as you did, but with port 8080.

I modified the example in goprocontrol.ino to free the dynamic allocation of memory from stringCut() in getMediaList() function. I alsow modified the example ArduinoJson.ino, the returned json is an array of object, not an object.

I created a function to set the connection , see setConnection() in goprocontrol.cpp. I use it to test in my code if the gopro is still connected. Sometimes it disconnect, don't know why, maybe I make requests too quick...

Maybe this can be lighter, I let you juge ! :)

Next step will be for getstatus, wich is not a json for hero3. I will make a new pull request !

damiendon commented 3 years ago

ok thank You Aster, i made the modifications. It's beginer mistakes i'm learning 😁 i will test it as soon as possible !

damiendon commented 3 years ago

ok it work but I can't use directly sendHTTPRequest(const char request, const uint16_t port = _wifi_port) I must do sendHTTPRequest(const char request, const uint16_t port = 80) in .h file, instead I have compilation errors.

aster94 commented 3 years ago

let's wait for the test and later we will discuss other small changes

aster94 commented 3 years ago

Hi @damiendon,

Have you had the time to test the changes?

damiendon commented 3 years ago

Hi @aster94 ! Yes, all the things i made this morning are working ! but still can't use _wifi_port... I add things for get status with hero3. the exemple is modified for hero3, can't test the json for others camera... i alsow add a function to calculate the length of the content in the response (extractResponselength()). It might be usefull to not over flow the buffer for getmedialist. but no time to implement. when there is to much media, buffer is not big enougth and esp8266 crash.

aster94 commented 3 years ago

Hi @damiendon thanks for the PR, I added some comments and suggestions

damiendon commented 3 years ago

Hi @aster94 . i made modifications, I'm still testing other things for medialist. For now if the message with the media list is too long, esp crash and restart because we don't verify if the buffer is big enougth. Sorry, i'm still discovering new things, it takes time for me to test it !

aster94 commented 3 years ago

Don't worry Damien, take all the time you need to discover and learn 😀 ESP usually crash for memory leakage, you may check it in the loop of the function the available memory with ESP.getFreeHeap() If you wish to stop the leakage set a max char dimension for your variable and if extractResponselength() give you a char too long just write to the user something like "medialist too long, printing until __ character", this is just an idea, feel free to do as you wish

damiendon commented 3 years ago

Hi @aster94 , thank you for your last comment ! I think i'm done with it ! :) extractResponselength is used only for status (bytes for hero3), wich can't be treated as medialist because null. To avoid any problem with the buffer size (_response_buffer[1500];), I test in listenResponse() if available data are < size of buffer. Like this it's usefull for all others functions where we are waiting a response. I tested everything, several times, and all is working as expected without problem. It should be nice to test on others camera if it's working as expected.

Only one thing still not working and crash the ESP and the gopro, it's when I ask getMediaList() to run when gopro hero3 is off (wifi on). I must unplug ESP and gopro battery to retrieve a normal working. So i add a comment to not use getMediaList() when gopro is off ^^

One last thing to do (but on another PR), is to add in isOn() the real state of the HERO3 with getStatus()... I can't do it now, so will do it later.

Tell me if there are others modifications to do, I tried to write it as clearly as i could ! :)

damiendon commented 3 years ago

Hi @aster94 , did you have time to watch what I did ? :)

aster94 commented 3 years ago

Hi @damiendon I am really sorry to lost all this time between a review and the other one. To me almost everything seems good, I have only little corrections to point out, I am opening a new review

aster94 commented 3 years ago

I think is ok to merge 😁 I will post the new version soon

damiendon commented 3 years ago

Great ! Thank you for your time for reviewing my work ! 😁