search

tmiw / arduino-ntpd

NTP server for Arduino
34 stars 14 forks source link

Some modern C++ compliance issues #5

Closed markmac99 closed 5 years ago

markmac99 commented 5 years ago

I'd like to fix - however I don't have permission to create a branch Mainly the use of const strings where char arrays were expected (eg "/time" being passed where a char* or char[] was reqiured - C++ no longer permits this).

sww1235 commented 5 years ago

The usual method of contributing to a project on GitHub is to fork the repo and then create a pull request from a branch you create on your own fork.

Does this not work for you?

tmiw commented 5 years ago

There's another pull request in progress which, among other things, moves most of the static strings to flash memory. It may be worthwhile to wait until that's merged before attempting to update with your desired changes.

Also, what @sww1235 said.

markmac99 commented 5 years ago

@sww1235, yes, this works for me - thanks. I've not done this before though so i did not know how to do it. My previous experience of git/stash is in a commercial envt where you would just branch the code. @tmiw happy to wait. I'm also seeing some data corruption taking place in the data coming back from the GPS which i can "fix" by putting in Serial.print statements. I've found in the past that this often indicates memory overruns somewhere and I thought i had a fix but I'll wait to see what other fixes go in and then rebaseline my copy before submitting anything.

tmiw commented 5 years ago

Since that other pull request was submitted last year (oops!) I'm not sure we'll get a response to the comments quickly. I'll watch it over the next day or so and see. I may just approve it and resolve the (minor) comments myself if we don't hear back quickly.

Anyway, I'll let you guys know within a few days what happens regardless.

tmiw commented 5 years ago

The previously mentioned pull request has now been merged in, so feel free to make changes accordingly. :)

That said, it did seem to compile without issues in the 1.8.10 IDE (after including TinyGPS, of course). There was one warning, though, but it's in the Arduino Ethernet library:

`In file included from /Applications/Arduino.app/Contents/Java/libraries/Ethernet/src/Dns.cpp:8:0: /Applications/Arduino.app/Contents/Java/libraries/Ethernet/src/Dns.cpp: In member function 'uint16_t DNSClient::BuildRequest(const char*)': /Applications/Arduino.app/Contents/Java/libraries/Ethernet/src/utility/w5100.h:457:25: warning: result of '(256 << 8)' requires 18 bits to represent, but 'int' only has 16 bits [-Wshift-overflow=]

define htons(x) ( (((x)<<8)&0xFF00) | (((x)>>8)&0xFF) )

                  ~~~^~~

/Applications/Arduino.app/Contents/Java/libraries/Ethernet/src/Dns.cpp:164:18: note: in expansion of macro 'htons' twoByteBuffer = htons(QUERY_FLAG | OPCODE_STANDARD_QUERY | RECURSION_DESIRED_FLAG); ^~~~~ Sketch uses 29094 bytes (11%) of program storage space. Maximum is 253952 bytes. Global variables use 1123 bytes (13%) of dynamic memory, leaving 7069 bytes for local variables. Maximum is 8192 bytes. `

Also, the program in the test folder isn't compiling for me with the provided shell scripts due to missing Arduino.h and WProgram.h files. That should probably be fixed at some point (but you're welcome to take a shot at that, too).

markmac99 commented 5 years ago

Excellent, looks like the C++ issues are fixed :) I'm having some problems with buffer overruns on the GPS serial port, but I'll investigate those further. I'll close this thread and create a pull request if i have anything worth taking a look at!