provide full version descriptor, displayed in debug mode #4467

d-a-v · 2018-03-06T01:57:51Z

~~(wip)~~
This is intended to help users showing their running version when reporting.
The version string is given by ESP.getFullVersion()
Version is displayed if debug on serial is enabled ~~, or if Serial.setDebugOutput(true) is called~~.
Uses git describe --tags on linux, or ~~win-2.4.0-post~~ win-2.4.1-pre on windows (set inside platform.txt)

caveats:

~~Travis may not like core_version.h disappearance~~
~~some macro format are modified~~
~~global variable core_version removed~~
Strings may be too long, it currently shows:
- ~~Boot:31/~~SDK:2.2.1(cfd48f3)/Core:2.4.0-94-g7999f5c/lwIP:1.4.0rc2
- ~~Boot:31/~~SDK:2.2.1(cfd48f3)/Core:2.4.0-95-gd1f9c14/lwIP:2.0.3(STABLE-2_0_3_RELEASE/glue:arduino-2.4.0-5-gafb60c2)

igrr · 2018-03-06T02:56:57Z

cores/esp8266/Esp.cpp

+
+String EspClass::getFullVersion()
+{
+    return   String(F("Boot:")) + system_get_boot_version()


system_get_boot_version will not return any meaningful value, because we are using eboot instead of the SDK bootloader.

OK, will be removed

igrr · 2018-03-06T02:58:15Z

cores/esp8266/HardwareSerial.cpp

@@ -39,6 +39,9 @@ void HardwareSerial::begin(unsigned long baud, SerialConfig config, SerialMode m
 {
    end();
    _uart = uart_init(_uart_nr, baud, (int) config, (int) mode, tx_pin, _rx_size);
+#if defined(DEBUG_ESP_PORT) && !defined(NDEBUG)
+    println(ESP.getFullVersion());


Usually after reset you get some garbage, so a newline might be necessary before the version string.

igrr · 2018-03-06T03:00:00Z

cores/esp8266/core_esp8266_main.cpp

@@ -41,6 +41,7 @@ extern "C" {

 struct rst_info resetInfo;

+#if 0
 extern "C" {
 extern const uint32_t __attribute__((section(".ver_number"))) core_version = ARDUINO_ESP8266_GIT_VER;


If you remove this, then eboot will no longer be able to print the core version:

Arduino/bootloaders/eboot/eboot.c

Lines 21 to 33 in 7999f5c

int print_version(const uint32_t flash_addr)

{

uint32_t ver;

if (SPIRead(flash_addr + APP_START_OFFSET + sizeof(image_header_t) + sizeof(section_header_t), &ver, sizeof(ver))) {

return 1;

}

const char* __attribute__ ((aligned (4))) fmtt = "v%08x\n\0\0";

uint32_t fmt[2];

fmt[0] = ((uint32_t*) fmtt)[0];

fmt[1] = ((uint32_t*) fmtt)[1];

ets_printf((const char*) fmt, ver);

return 0;

}

(which i personally use to check that I'm running correct version; but if you think it should be removed i can find some other way)

I personally don't mind, I could not find where it was used in the core.

igrr · 2018-03-06T03:03:34Z

cores/esp8266/HardwareSerial.cpp

@@ -94,6 +97,9 @@ void HardwareSerial::setDebugOutput(bool en)
    if(en) {
        if(uart_tx_enabled(_uart)) {
            uart_set_debug(_uart_nr);
+#if !defined(DEBUG_ESP_PORT) && !defined(NDEBUG)
+            os_printf("%s\r\n", ESP.getFullVersion().c_str());
+#endif


I'm fine with printing the version in Serial::begin if debug output is enabled... however if i'm calling setDebugOutput i probably know what i'm doing and can handle calling ESP.getFullVersion if needed.

Removing this one will also allow the linker to get rid of the code if sources are moved to a separate file.

igrr · 2018-03-06T03:04:02Z

cores/esp8266/core_esp8266_main.cpp

@@ -150,7 +152,7 @@ void init_done() {
    system_set_os_print(1);
    gdb_init();
    do_global_ctors();
-    printf("\n%08x\n", core_version);
+    //printf("\n%08x\n", core_version);


This printf seems to be some debugging leftover — it is not used and can be removed.

igrr · 2018-03-06T03:04:49Z

platform.txt

@@ -6,7 +6,7 @@
 # https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5---3rd-party-Hardware-specification

 name=ESP8266 Modules
-version=2.5.0
+version=2.4.0-post


According to semver, 2.4.0-post is something before 2.4.0.

It might be 2.4.1-pre if we intend to merge this before 2.4.1?

igrr · 2018-03-06T03:07:16Z

platform.txt

-recipe.hooks.core.prebuild.1.pattern=bash -c "mkdir -p {build.path}/core && echo \#define ARDUINO_ESP8266_GIT_VER 0x`git --git-dir {runtime.platform.path}/.git rev-parse --short=8 HEAD 2>/dev/null || echo ffffffff` >{build.path}/core/core_version.h"
-## windows-compatible version may be added later
-recipe.hooks.core.prebuild.1.pattern.windows=
+recipe.hooks.core.prebuild.1.pattern=bash -c "mkdir -p {build.path}/core && echo \#define ARDUINO_ESP8266_GIT_VER `cd {runtime.platform.path}; git describe --tags 2>/dev/null || echo nix-{version}` >{build.path}/core/core_version.h"


Can we keep ARDUINO_ESP8266_GIT_VER to be a short hash (expressed as an integer), and add a new define for the git describe output instead? E.g. ARDUINO_ESP8266_GIT_DESC. I have some release scripts which rely on ARDUINO_ESP8266_GIT_VER being what it is.

Also, not necessarily nix- version. Also works on macOS, as far as i can tell.

Will rename to GIT_DESC, and leave GIT_VER as before. About nix, it's a typo, unix was intended (for linux + macos).

igrr · 2018-03-06T03:12:23Z

I think the idea with core_version.h was that the generated file would be placed into the build directory, so that the compiler picks it up before the one in the core. If this is broken for some reason, maybe it means that the IDE has started calling arduino-builder in some different way...

devyte · 2018-03-06T04:54:07Z

@igrr maybe that's the reason for #3657 ? I had to delete core_version.h in my local repo to get the version to work correctly.

d-a-v · 2018-03-06T14:16:33Z

I hope we have a better view of what's happening in issues after next release.

devyte · 2018-03-06T20:19:05Z

Some questions on this:

is a separate file for this really necessary? I'd prefer to stick to 1-cpp-per-object-definition.
This code introduces an entirely new way of retrieving versions, which is independent of the existing ones, and has duplication of function. Rather than implement a new way, does it make sense to enhance the current Esp::getBlahVersion() methods, add a new one for lwip, and implement this new Esp::getFullVersion() as a function of the others?
Why is this available only when building without NDEBUG? It seems inconsistent wrt the other Esp::getBlahVersion() methods, which are always available.

devyte · 2018-03-06T20:20:42Z

@igrr if system_get_boot_version() doesn't return something meaningful, shouldn't we re-implement EspClass::getBootVersion() in terms of the current eboot version?

d-a-v · 2018-03-06T23:11:30Z

is a separate file for this really necessary? I'd prefer to stick to 1-cpp-per-object-definition.

If this function is not used, then it takes no flash (~400bytes just to print a ~40bytes string) even if another EspClass:: one is used. It's because we don't have -flto yet.

This code introduces an entirely new way of retrieving versions, which is independent of the existing ones, and has duplication of function. Rather than implement a new way, does it make sense to enhance the current Esp::getBlahVersion() methods, add a new one for lwip, and implement this new Esp::getFullVersion() as a function of the others?

To be honest, I'm quite bothered when it comes to string manipulations and all those calls to malloc making tiny holes in ram tickle me. That's the reason why I enclosed them in a big function. It's rather useless, but it's all put in a single place. Still you're right, let's keep the modular way and let's expect gcc5 (or is it gcc49?) to come soon!
(: Or, we may use a big goodold C snprintf :)

Why is this available only when building without NDEBUG? It seems inconsistent wrt the other Esp::getBlahVersion() methods, which are always available.

I put this NDEBUG conditional when the function was in Esp.cc because people using this option want to save more flash. It makes indeed no sense to keep it in the separate file.

igrr

LGTM except for these two minor things!

igrr · 2018-03-07T10:00:35Z

cores/esp8266/Esp-version.cpp

@@ -0,0 +1,46 @@
+


License header missing here

igrr · 2018-03-07T10:04:25Z

cores/esp8266/HardwareSerial.cpp

@@ -39,6 +39,10 @@ void HardwareSerial::begin(unsigned long baud, SerialConfig config, SerialMode m
 {
    end();
    _uart = uart_init(_uart_nr, baud, (int) config, (int) mode, tx_pin, _rx_size);
+#if defined(DEBUG_ESP_PORT) && !defined(NDEBUG)
+    println();
+    println(ESP.getFullVersion());


Unrelated to printing the version, but since you're adding this code block here. Let's call setDebugOutput(true); or the equivalent uart_ function. At the moment debug output only gets enabled before loop runs, so you don't get debug output while setup is running. Then these three lines can be removed:

Arduino/cores/esp8266/core_esp8266_main.cpp

Lines 118 to 120 in 7999f5c

#ifdef DEBUG_ESP_PORT

DEBUG_ESP_PORT.setDebugOutput(true);

#endif

To understand well, setDebugOutput(true) (= sdk debug messages) is then not automatically enabled anymore when Serial debug is enabled, we would not have espressif debug messages anymore, but only this "full version" displayed and selected arduino-core debug messages ?
For anywone willing to get sdk debug messages, then setDebugOutput(true) has to be called.

That sounds nice to me.

I'm suggesting that we move the call to setDebugOutput(true); a bit earlier, if DEBUG_ESP_PORT is set.
Currently it happens only before loop, so some debug messages during setup may be lost.
If we do this in HardwareSerial::begin, then we will see all debug messages. So the feature merely becomes more consistent.

(We also need if (&DEBUG_ESP_PORT == this) { check before enabling debug output, in case DEBUG_ESP_PORT is the HardwareSerial which we are initializing).

Ok to summarize, we should automatically enable sdk messages along with core messages, but before setup not after.

OK about Serial/Serial1 distinction.

igrr · 2018-03-07T14:49:50Z

package/build_boards_manager_package.sh

@@ -88,6 +88,7 @@ $SED 's/recipe.hooks.core.prebuild.1.pattern.*//g' \
 ver_define=`echo $plain_ver | tr "[:lower:].\055" "[:upper:]_"`
 echo Ver define: $ver_define
 echo \#define ARDUINO_ESP8266_GIT_VER 0x`git rev-parse --short=8 HEAD 2>/dev/null` >$outdir/cores/esp8266/core_version.h
+echo \#define ARDUINO_ESP8266_GIT_DESC `git describe --tags 2>/dev/null` >>$outdir/cores/esp8266/core_version.h


A few lines above (can't comment on the line directly...) there is

$SED 's/recipe.hooks.core.prebuild.1.pattern.*//g' \

Since you have added 2.pattern, please add one more SED line to remove 2.pattern, then run the script and check that the prebuild hooks are removed from platform.txt in the built archive.

Done. I had to rebase to push the fix.

also: improve version string remove useless message

restore ARDUINO_ESP8266_GIT_VER restore global variable "core_version" don't print full version on setDebugOutput(true) set platform.txt version to 2.4.1-pre hide irrelevant boot version fix typo

… setup not after

…bler in hardware serial

igrr reviewed Mar 6, 2018

View reviewed changes

igrr approved these changes Mar 7, 2018

View reviewed changes

igrr requested changes Mar 7, 2018

View reviewed changes

d-a-v added 16 commits March 7, 2018 16:05

lwip2: fix disconnection/reconnection issue

a0d16f6

also: improve version string remove useless message

lwip2: bump tag before 2.4.1

8b57529

lwip2: improve netif flags management on git side

92b360b

provide full version descriptor, displayed in debug mode

ae0258e

unix: shows core version like under windows when git is unavailable

74aab7e

store strings in progmem

dae3932

version string honours NDEBUG

dbd0126

add ARDUINO_ESP8266_GIT_DESC

1dc99d6

restore ARDUINO_ESP8266_GIT_VER restore global variable "core_version" don't print full version on setDebugOutput(true) set platform.txt version to 2.4.1-pre hide irrelevant boot version fix typo

full-version string: remove useless NDEBUG in separate source file

613f873

do not automatically enable sdk messages along with core messages

6dc6514

automatically reenable sdk messages along with core messages *before*…

4eb01a1

… setup not after

check serial port when showing version-string + move sdk messages ena…

d55d501

…bler in hardware serial

+ license header

1e04a77

updated and tested windows commands in platform.txt (without git)

982c5a0

updated and tested windows commands in platform.txt (without git)

85945a1

update package builder accordingly

ee70fd1

d-a-v force-pushed the version branch from 816e209 to ee70fd1 Compare March 7, 2018 15:06

igrr approved these changes Mar 8, 2018

View reviewed changes

igrr merged commit 8053f28 into esp8266:master Mar 8, 2018

d-a-v deleted the version branch March 8, 2018 13:59

bryceschober mentioned this pull request Apr 5, 2018

Allow other ESP debug port class types #4611

Merged

d-a-v mentioned this pull request Sep 30, 2018

Core & Wifi debug output sent to serial port despite "None" Debug Level in Arduino's IDE #5186

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide full version descriptor, displayed in debug mode #4467

provide full version descriptor, displayed in debug mode #4467

d-a-v commented Mar 6, 2018 •

edited

Loading

igrr Mar 6, 2018

d-a-v Mar 6, 2018

igrr Mar 6, 2018

igrr Mar 6, 2018

igrr Mar 6, 2018

d-a-v Mar 6, 2018

igrr Mar 6, 2018

d-a-v Mar 6, 2018

igrr Mar 6, 2018 •

edited

Loading

igrr Mar 6, 2018

igrr Mar 6, 2018

igrr Mar 6, 2018

d-a-v Mar 6, 2018

igrr commented Mar 6, 2018

devyte commented Mar 6, 2018

d-a-v commented Mar 6, 2018

devyte commented Mar 6, 2018

devyte commented Mar 6, 2018

d-a-v commented Mar 6, 2018

igrr left a comment

igrr Mar 7, 2018

d-a-v Mar 7, 2018

igrr Mar 7, 2018

d-a-v Mar 7, 2018 •

edited

Loading

igrr Mar 7, 2018

d-a-v Mar 7, 2018

igrr Mar 7, 2018 •

edited

Loading

d-a-v Mar 7, 2018

	int print_version(const uint32_t flash_addr)
	{
	uint32_t ver;
	if (SPIRead(flash_addr + APP_START_OFFSET + sizeof(image_header_t) + sizeof(section_header_t), &ver, sizeof(ver))) {
	return 1;
	}
	const char* __attribute__ ((aligned (4))) fmtt = "v%08x\n\0\0";
	uint32_t fmt[2];
	fmt[0] = ((uint32_t*) fmtt)[0];
	fmt[1] = ((uint32_t*) fmtt)[1];
	ets_printf((const char*) fmt, ver);
	return 0;
	}

	#ifdef DEBUG_ESP_PORT
	DEBUG_ESP_PORT.setDebugOutput(true);
	#endif

provide full version descriptor, displayed in debug mode #4467

provide full version descriptor, displayed in debug mode #4467

Conversation

d-a-v commented Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr commented Mar 6, 2018

devyte commented Mar 6, 2018

d-a-v commented Mar 6, 2018

devyte commented Mar 6, 2018

devyte commented Mar 6, 2018

d-a-v commented Mar 6, 2018

igrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-a-v Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-a-v commented Mar 6, 2018 •

edited

Loading

igrr Mar 6, 2018 •

edited

Loading

d-a-v Mar 7, 2018 •

edited

Loading

igrr Mar 7, 2018 •

edited

Loading