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.

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

Beta #462

Merged
merged 42 commits into from
Sep 11, 2019
Merged

Beta #462

merged 42 commits into from
Sep 11, 2019

Conversation

nospam2000
Copy link
Contributor

ESP32 related fixes

now = time(nullptr);
#endif
#if defined(ESP32)
if (now > 17897*24*60*60) { // later than 2019/01/01:00:00:00
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't that mean that sntp_time_is_set should not be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I understood your questions correctly.
sntp_time_is_set is initialized with false and only set in the callback time_is_set().
When the callback is called in the first try with "ntp.org", the second part (the change from line 4247 to 4252) will never be reached because the function returns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I agree with your assessment, sorry, didn't read the code closely. I wonder if we can remove all of the ESP conditionals in the code and use both code paths the same way?

Copy link
Contributor Author

@nospam2000 nospam2000 Sep 9, 2019

Choose a reason for hiding this comment

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

Yes, in this specific case the ESP32 path will work for both. The error checking of the ESP8266 is more strict. The fallback in the ESP32 case to use the local router will not be triggered if the RTC has a date after 2019-01-01 and this will happen on a reboot.

That means:

  • when ntp.org cannot be reached on the first boot it will fallback to the local router
  • on the second boot the RTC will already have a valid date and don't do a fallback to the local router. /edit: this means the time will not by synchronized any longer and start to drift

To solve that, I would remove the fallback and replace it by the following solution:
Set multiple servers with one call:

  1. ntp.org
  2. the ntp server(s) announced by dhcp
  3. the local router (actually this shall not be necessary because in many cases this is already covered by 2.)

roughly the code would be the following, but I need to check how to mix dhcp and fixed servers:

sntp_servermode_dhcp(1);
sntp_setservername(0, "pool.ntp.org");
sntp_setserver(1, WiFi.gatewayIP());

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would try local first before going to the internet.. I implemented router/pool in my pr that I just opened.

Searching for ntp information in dhcp does sound like a good idea though.

now = time(nullptr);
#endif
#if defined(ESP32)
if (now > 17897*24*60*60) { // later than 2019/01/01:00:00:00
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is sntp_time_is_set now only used for ESP8266?

Copy link
Contributor Author

@nospam2000 nospam2000 Sep 8, 2019

Choose a reason for hiding this comment

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

because settimeofday_cb() is not available for ESP32. The examples are using sntp_set_time_sync_notification_cb() but this is not yet available in the newest esp32platform delivered with platformio.

/edit: see also https://github.com/espressif/esp-idf/tree/master/examples/protocols/sntp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, thanks

@dirkmueller
Copy link
Collaborator

@nospam2000 hah, we were looking at the same problem! I opened #463 for this, it reduces the code bloat a bit as well.

@nospam2000
Copy link
Contributor Author

we were looking at the same problem!

@dirkmueller how shall we synchronize to avoid duplicate work? Shall we use github issues or the ESP32 chat on luftdaten.info?

@dirkmueller
Copy link
Collaborator

@nospam2000 yes, we should coordinate. can you point me to the place where to find information about this "chat" on luftdaten.info ?

@nospam2000
Copy link
Contributor Author

I was invited to the chat by (martin ät luftdaten Punkt info), but I think I can also invite you, if you give me your email address. My address is (nospam2000 ät gmx Punkt de).

using 'lib_ldf_mode = off' because ldf doesn't work correctly for #ifdef'ed includes
added config for ESP32
using newest possible ESPSoftwareSerial
and why which variant is needed
extra_scripts = platformio_script.py
# This release is reflecting the Arduino Core 2.4.2 release
# When the requirement for Arduino Core is raised, this
# needs to be adjusted to the matching version from
# https://github.com/platformio/platform-espressif8266/releases
platform_version = [email protected]
;platform_version = [email protected]
platform_version = [email protected] ; using Arduino core 2.5.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

please update the comment above - but we can't upgrade to newer arduino core until OTA problem is solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already reverted. I have to switch back and forth all the time...

I'm not sure about using which SoftwareSerial variant and version.

Copy link
Contributor

Choose a reason for hiding this comment

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

What OTA problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

firmware size>512kB cannot be flashed using OTA:
4 MB Flash 3MB SPIFFS, 512 kB running firmware, 512kB new firmware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way: what is the real use of mDNS? Removing it would free some flash and the old version is pretty buggy

Copy link
Contributor

Choose a reason for hiding this comment

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

I often use it to quickly see the current PM value of my own sensor.

mDNS assigns a name to the node, but also adverttises the services running on the node (e.g. webserver, even the Arduino OTA port if you have local OTA enabled). On Android you can see this with https://play.google.com/store/apps/details?id=com.druk.servicebrowser for example, on linux you can see local services with avahi-discover,

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nospam2000 the name can be discovered in the initial setup step where you connect to it in ap mode. Also I thought you can configure it..

Copy link
Member

Choose a reason for hiding this comment

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

mDNS is used to discover available dust sensors with the firmware flashing tool (https://github.com/opendata-stuttgart/airrohr-firmware-flasher). So usersdon't need to know how to look up the IP address (this project is also for users without much IT knowlegde).
DNSServer is needed for the "login" feature while setting up the sensor. Most OSes send a DNS request for special host names to test internet connectivity. These requests need to be answered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The description of the firmware flasher says "connect the ESP8266 with a USB cable to your computer (micro-USB)". Can it really also be used for OTA flashing as a local OTA update? Actually that is what I was looking for because the USB upload is a pain.

Copy link
Member

Choose a reason for hiding this comment

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

Not at the moment. Maybe we can implement something similar with the update version @dirkmueller is working on (load new firmware to SPIFFS, update actual firmware to a small binary that will then only copy the new version from SPIFFS to flash)

@nospam2000
Copy link
Contributor Author

The build of the ESP32 target didn't succeed, probably because PlatformIO is too old.
Is it possible to switch to a newer PlatformIO version when the esp8266 platform version is now fixed at 1.8.0?

@dirkmueller
Copy link
Collaborator

Yes, we can remove the cap on platformio to go with a newer version

@ricki-z
Copy link
Member

ricki-z commented Sep 11, 2019

@nospam2000 could you please rebase your commit? There were some changes by an earlier commit that have some conflicts with your changes.

@nospam2000
Copy link
Contributor Author

nospam2000 commented Sep 11, 2019

Yes I already rebased, but the original beta branch and therefore now also my branch doesn't compile any longer with ESP32 with the following error (for every use of the debug functions):

Compiling .pio/build/heltec_wifi_lora_32_V2/src/airrohr-firmware.ino.cpp.o
airrohr-firmware.ino: In function 'String SDS_version_date()':
airrohr-firmware.ino:848:84: error: no matching function for call to 'debug_outln_verbose(const char*, const char*)'
  debug_outln_verbose(FPSTR(DBG_TXT_END_READING), FPSTR(DBG_TXT_SDS011_VERSION_DATE));
                                                                                    ^
airrohr-firmware.ino:619:13: note: candidate: void debug_outln_verbose(const __FlashStringHelper*)
 static void debug_outln_verbose(const __FlashStringHelper* text) {
             ^
airrohr-firmware.ino:619:13: note:   candidate expects 1 argument, 2 provided
airrohr-firmware.ino:629:13: note: candidate: void debug_outln_verbose(const __FlashStringHelper*, const String&)
 static void debug_outln_verbose(const __FlashStringHelper* text, const String& option) {
             ^
airrohr-firmware.ino:629:13: note:   no known conversion for argument 1 from 'const char*' to 'const __FlashStringHelper*'

lib_ldf_mode = chain+ for ESP8266 because 'off' doesn't work with SoftwareSerial
lib_ldf_mode = off for ESP32
Copy link
Member

@ricki-z ricki-z left a comment

Choose a reason for hiding this comment

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

the upload and monitor ports are not the mentioned on all systems. They are different between OSes and they may even differ between different modules (depending on the used serial-2-usb chipset).
So we should avoid setting this.

@nospam2000
Copy link
Contributor Author

the upload and monitor ports are not the mentioned on all systems. They are different between OSes

I will remove them.

The main problem is, that platformio has all configuration in one file: site/user specific and compile/linker settings.
At the moment I have to switch back and forth before every check-in to keep that config clear from local settings. Maybe I can use environment variables to overcome this, but I think there is also no #if mechanism in platformio.ini.

@nospam2000
Copy link
Contributor Author

Yes I already rebased, but the original beta branch and therefore now also my branch doesn't compile any longer with ESP32

The reason is this fix espressif/arduino-esp32#1371

@ricki-z
Copy link
Member

ricki-z commented Sep 11, 2019

By removing 'common includes' like <Adafruit_BMP085.h> the source code can't be compiled with the Arduino IDE.
The directory .vscode should be included in .gitignore.

@nospam2000
Copy link
Contributor Author

By removing 'common includes' like <Adafruit_BMP085.h> the source code can't be compiled with the Arduino IDE.

I just moved these includes down into a section shared by ESP8266 and ESP32. We should keep the differences between both architectures as small as possible and the external libraries are the same for both. Arduino IDE 1.8.9 compiles it for ESP8266 and also ESP32 (using newest libraries).

The directory .vscode should be included in .gitignore.

The ESP32 can do hardware JTAG debugging. For debugging (without PlatformIO-Plus) this hand-written launch.json file is needed. Actually PlatformIO will overwrite it with its own content, so it has to be write protected or just checked out again.
I tried to keep the content as generic as possible.

@ricki-z
Copy link
Member

ricki-z commented Sep 11, 2019

Includes: Okay I have found the second block (ex-ESP32). The source code folding here on Github is sometimes 'suboptimal'.

@ricki-z ricki-z merged commit 8149968 into opendata-stuttgart:beta Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants