Blocking AM2321 Library with Arduino 1.8.2 / Teensyduino 1.36

[sigi]

Hi,

i run a weather station since several years. Suddenly the startup did not work anymore. The debugging infos in startup over serial do not come. After several hours of debugging i could find that the initialisation of the AM2321 caused this. The code did not changed. Normally i simply search for an alternative library, but found none. Now it seems that i am forced to change the sensor or find the problem. The sensor must not be attached, and this worked some Arduino versions ahead. Code itself did not change.
Any idea is very welcome.

Libraries needed to compile:
https://github.com/wangdong/AM2321
https://github.com/geneReeves/ArduinoStreaming

// Test serial output at startup

#include <Streaming.h>
#include <AM2321.h>
int led1 = 13;

AM2321 ac;  // ===> comment to unblock

void setup()

{
  Serial.begin(115200); // USB is always 12 Mbit/sec
  pinMode(led1, OUTPUT);
  delay(100);
  Serial << (F("=========MeteoStation 100ms =========")) << endl;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial << (F("=========MeteoStation 200ms =========")) << endl;
  delay(100);
  digitalWrite(led1, LOW);
  Serial << (F("=========MeteoStation 300ms =========")) << endl;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial << (F("=========MeteoStation 400ms =========")) << endl;
  delay(100);
  digitalWrite(led1, LOW);
  Serial << (F("=========MeteoStation 500ms =========")) << endl;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial << (F("=========MeteoStation 600ms =========")) << endl;
  delay(100);
  digitalWrite(led1, LOW);
  Serial << (F("=========MeteoStation 700ms =========")) << endl;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial << (F("=========MeteoStation 800ms =========")) << endl;
  delay(100);
  digitalWrite(led1, LOW);
  Serial << (F("=========MeteoStation 900ms =========")) << endl;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial << (F("=========MeteoStation 1000ms =========")) << endl;
  delay(100);
  digitalWrite(led1, LOW);
 
}

void loop()
{
}

And i found an older post here that involved also AM2321 https://forum.pjrc.com/threads/31249...ll=1#post87010

[defragster]

There was a change to the behavior of Serial.begin() and entry time for USB starting in TD_1.36.

What happens if you change setup() to start like this?

void setup()
{
  delay(400);
  Serial.begin(115200); // USB is always 12 Mbit/sec
  delay(400);
// ... continue with setup code here

I can't test with what you have in hardware. Only one of those delays may be needed and it may not need to be that large. But if it works you can go from there.

[sigi]

You need no sensor for testing this, just Teensy3.x
It seems USB startup is not only delayed, it is blocked. Tested up to 30s. On Arduino Mega this code still runs with AM2321 ac; active

[defragster]

The entry to setup was adjusted for serial to be online faster - but if SerMon not ready it can then wait for only up to 1 second before returning - wait would be 600ms after setup entry - that is time for an active SerMon to connect or it starts without it.

You need no sensor for testing this, just Teensy3.x
It seems USB startup is not only delayed, it is blocked. Tested up to 30s. On Arduino Mega this code still runs with AM2321 ac; active

Neglected to edit this before posting :: I can't test with what you have in LIBRARIES/hardware.

If the delay(400)'s didn't change it - perhaps it is the compiler build optimization or toolchain updates that came with TD_1.36.

Bottom line - I cannot compile the code above so I can't help more. With normal Serial.print( "Hello World!" ); output this should work fine. You can test that instead of the CPP "<<" and it should work. Teensy_3's have no need for the F() macro.

I can comment out the obvious stuff needed to resolve the issues below - but would still not have a working sketch without more fixes/rewrites - and then I'm no where near testing the code presented.

I did that on another sketch and I saw and fixed a compiler warning and so the problem went away - it turns out the compiler warning indicated the problem with the code on that post. Check verbose output for any warnings. The build toolchain was updated - new warnings and compile/link operations could cause trouble - as well as the chosen build optimization level noted above.

I:/Code/FORUM/RAND/RAND.ino:17:23: fatal error: Streaming.h: No such file or directory
I:/Code/FORUM/RAND/RAND.ino:18:20: fatal error: AM2321.h: No such file or directory
I:/Code/FORUM/RAND/RAND.ino:21:1: error: 'AM2321' does not name a type
I:/Code/FORUM/RAND/RAND.ino:29:60: error: 'endl' was not declared in this scope
I:/Code/FORUM/RAND/RAND.ino:35:10: error: no match for 'operator<<' (operand types are 'usb_serial_class' and 'const __FlashStringHelper*')

[sigi]

T[U]I can't test with what you have in LIBRARIES/hardware.

Thanks for your efforts. The Link to the AM Library was in the first post and for "Streaming.h" i thought everybody uses this. My bad.. ;-) I striped the example code to the bare minimum to reproduce the problem.
This 2 Libraries are needed for compile

https://github.com/geneReeves/ArduinoStreaming
https://github.com/wangdong/AM2321

[sigi]

Removed streaming library for simpler test

#include <AM2321.h>
int led1 = 13;

AM2321 ac;  //Uncomment to unblock

void setup()
{
  Serial.begin(115200); // USB is always 12 Mbit/sec
  pinMode(led1, OUTPUT);
  delay(100);
  Serial.println (F("=========MeteoStation 100ms =========")) ;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial.println(F("=========MeteoStation 200ms =========")) ;
  delay(100);
  digitalWrite(led1, LOW);
  Serial.println(F("=========MeteoStation 300ms =========")) ;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial.println(F("=========MeteoStation 400ms =========")) ;
  delay(100);
  digitalWrite(led1, LOW);
  Serial.println(F("=========MeteoStation 500ms =========")) ;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial.println(F("=========MeteoStation 600ms =========")) ;
  delay(100);
  digitalWrite(led1, LOW);
  Serial.println(F("=========MeteoStation 700ms =========")) ;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial.println(F("=========MeteoStation 800ms =========")) ;
  delay(100);
  digitalWrite(led1, LOW);
  Serial.println(F("=========MeteoStation 900ms =========")) ;
  delay(100);
  digitalWrite(led1, HIGH);
  Serial.println(F("=========MeteoStation 1000ms =========")) ;
  delay(100);
  digitalWrite(led1, LOW);


}

void loop()
{

}

This also proves that USB init need around 400ms as you mentioned

[defragster]

Paul: Please look below - there is a problem with this object creation calling Wire.begin() differently. It isn't from the change to timing of usb_init as I restored and doubled the delay(350). The odd thing is that compiling with LTO must re-order when the object is created?

First thing I did was try to see blink right away. There is no entry to setup() at all - even to turn on the LED - until this line is commented: //AM2321 ac;

It is correct there is trouble with this library causing the Teensy to ABEND - but the trouble is not with Serial.begin() but before.

I made this change to determine there is no sign of usable entry to setup()::

void setup()
{
  pinMode(led1, OUTPUT);
  digitalWrite(led1, HIGH);
  Serial.begin(115200); // USB is always 12 Mbit/sec
  Serial.println( millis());
// ...

This failure is with default Optimize 'Faster'

As suggested above changing the compile optimization quickly allows it to run when set to 'Smallest code with LTO' or 'Faster with LTO'

When I change to 'Smallest code' it again no longer enters setup and turns on the LED.

@sigi :: Please compile your existing code "with LTO" and report your findings if it is fully functional. You might want to make the edit above to setup() to get the LED powered quickly to see signs of life.

There is something wrong here when non-LTO compiles are selected based on this limited test case. I changed the timing before the call { to setup() } to this ridiculous wait and it still will not function:

	delay(50);
	delay(350); // ADDED
	delay(350); // ADDED
	usb_init();
	delay(350);

If compiled 'LTO' the TD_1.36 default timing on entry to setup I see is 410 ms or less on my system with a T_3.1. { and it works }

Looking a bit to see what AM2321 might do I find it does a Wire.begin() on creation.

I made these changes and it now starts up fine with 'smallest' or 'Faster' - so the trouble is that instantiating the object "ac" is hitting 'Wire.begin() badly?

AM2321 ac;  //Uncomment to unblock
#include <Wire.h> // ADDED

void setup()
{
  pinMode(led1, OUTPUT);
  digitalWrite(led1, HIGH);
  Serial.begin(115200); // USB is always 12 Mbit/sec
  delay(100);
  Wire.begin(); // ADDED
  digitalWrite(led1, LOW);
  Serial.println (F("=========MeteoStation 100ms =========")) ;

And in the library code I:\Code\libraries\AM2321\AM2321.cpp:

AM2321::AM2321() {
    //Wire.begin(); // COMMENTED
    temperature = 0;
    humidity    = 0;
}

NOTE: I also added this and it had No effect - even on the time of entry to setup()::

void startup_late_hook(void) {
  delayMicroseconds(100000);
  delayMicroseconds(100000);
  delayMicroseconds(100000);
  delayMicroseconds(100000);
}

[sigi]

Wow! Thanks! Fast with LTO seems to work. Also serial init is much faster. From between 300ms and 400ms to sub 100ms. But cannot fully test it for now. My lab is outdoors at 2250m ASL ;-) I also forgot to mention that i run only on 48Mhz. It is the first time since years, i had a real issue with teensyduino.

[defragster]

Glad that helped! LTO has been seen to break things - only the second time I've seen it HELPED things.

Question: since you didn't run up the mountain to test, does that mean you are running this with no sensor attached? That might indicate something useful - and what happens when the device is present may be different.

Paul made some changes to Wire.h - odd but possible it is that getting caught in the build triggering a fault case. Based on my test I don't see it being because of the faster USB startup as I reverted the change I had suggested Paul might make.

[sigi]

Question: since you didn't run up the mountain to test, does that mean you are running this with no sensor attached? That might indicate something useful - and what happens when the device is present may be different.

Yes, as i mentioned in the first post: There is no hardware needed to test this issue ;-)
At least the startup and watchdog setup should work after new programming.
Since my station is still beta, i do not have the complete hardware here to test. But it is also important to me, that a single failing sensor cannot break whole system which sends 25 different data types regularly.

[tni]

Construction order of global variables is only defined within the same translation unit (.cpp file). So global objects defined in different translations units using each other is a very bad idea - "Wire" is defined in a different translation unit than "AM2321 am".

"Wire.begin()" uses a "hardware" member containing the I2C register addresses, that may not be initialized when it's used from a global variable.

It's easy to see, if you make the TwoWire data members public ("arduino/hardware/teensy/avr/libraries/Wire/WireKinetis.h"):

#include <Wire.h>
volatile uint32_t cgr_addr = (uint32_t)&Wire.hardware.clock_gate_register;

void setup() {
  Serial.begin(115200);

  Serial.print("CGR addr (static initialization): ");
  Serial.println(cgr_addr, HEX);

  Serial.print("CGR addr (setup): ");
  Serial.println((uint32_t)&Wire.hardware.clock_gate_register, HEX);
}

void loop(){}

No LTO:
CGR addr (static initialization): 20008000
CGR addr (setup): 40048034

LTO:
CGR addr (static initialization): 40048034
CGR addr (setup): 40048034

LTO can change the static initialization ordering between translations units, that's why things are working with LTO.

[KurtE]

Personally instead of relying on hacks to get the code to work, I would update the library to not have the Wire.begin as part of the constructor, but instead move it to a secondary place. Like add a member called begin or init..

Or alternatively add a flag that says it has not been init and call Wire.begin internal to other methods... Or just tell the user to add Wire.begin to their own init code...

[defragster]

Thanks tni for demonstrating what I hinted at - good info. Sometimes 'with LTO' drops code - or as FrankB found will put it 'out of bounds'. In this case of NO LTO the code may be mis-associated in time or space of initialization.

This may show that it isn't Paul's recent Wire.h changes having any effect - I don't pretend to know the address space implications of 20008000 versus 40048034. Swapping in the TD_1.35 Wire.h could show that?

Personally instead of relying on hacks to get the code to work, ... Or just tell the user to add Wire.begin to their own init code...

I don't see a suggestion of trusting a hack to work - I noted it may not actually work as I wondered if it was running real hardware, but it was a simpler way to test than having sigi edit the library as I demonstrated { though if he is prepared to do this it might be needed to function } - which as you suggest would be best to not have such code on creation - which it seems Paul has noted as BAD before. Paul's one concern with getting into setup() too fast was hitting devices not ready - like i2c - thus my interest in this.

I expected my post to be less than half that length ( and one third the effort ) as it was past bedtime. I only wanted to prove that setup timing from my suggested usb_init() call change wasn't at fault. Then I opened the library code and saw the .begin() there and had to start over to see it wasn't just startup timing.

Question: Why didn't startup_late_hook() code above have the expected effect of slowing the entry to setup() - am I missing something? I was hoping this might offer a workaround if startup timing really was

NOTE: Doing this I can't recommend SublimeText with Stino highly enough when a more powerful IDE is appropriate! {in conjuction with TyCommander } I did this without using the IDE! Stino builds using the installed IDE and TD_1.36, and SublimeText GROKS the whole of the TeensyDuino library tree allowing global Search to find and edit files - all within the same very helpful tool!

[tni]

Thanks tni for demonstrating what I hinted at - good info. Sometimes 'with LTO' drops code - or as FrankB found will put it 'out of bounds'. In this case of NO LTO the code may be mis-associated in time or space of initialization.

This may show that it isn't Paul's recent Wire.h changes having any effect - I don't pretend to know the address space implications of 20008000 versus 40048034. Swapping in the TD_1.35 Wire.h could show that?

Paul's changes from Mar 12 introduced this problem. Before, the register stuff was hard-coded. Now there is a member with the register addresses that needs to be initialized.

"20008000" is a garbage value - the TwoWire constructor hasn't run yet. Using that as register address causes the CPU to hang.

Question: Why didn't startup_late_hook() code above have the expected effect of slowing the entry to setup() - am I missing something?

Yes. "startup_late_hook()" runs after global variable initialization, it's already too late.

There is an idiom called "Nifty Counter" to ensure initialization of a global variable before before it's first used. Something like that is usually used for "cout".

This fixes the initialization:

In WireKinetis.h:

#ifdef WIRE_IMPLEMENT_WIRE
//extern TwoWire Wire;
extern TwoWire& Wire;
static struct WireInitializer {
  WireInitializer ();
  ~WireInitializer ();
} wireInitializer; // static initializer for every translation unit
#endif

In WireKinetis.cpp:

#include <new>
#include <type_traits> 

...

#ifdef WIRE_IMPLEMENT_WIRE
//TwoWire Wire(KINETIS_I2C0, TwoWire::i2c0_hardware);
static int wire_counter;
static typename std::aligned_storage<sizeof (TwoWire), alignof (TwoWire)>::type
  wire_buf; // memory for the Wire object
TwoWire& Wire = reinterpret_cast<TwoWire&> (wire_buf);
WireInitializer::WireInitializer () {
  if (wire_counter++ == 0) new (&Wire) TwoWire(KINETIS_I2C0, TwoWire::i2c0_hardware); // placement new
}
WireInitializer::~WireInitializer () {
  if (--wire_counter == 0) (&Wire)->~TwoWire();
}
void i2c0_isr(void) { Wire.isr(); }
#endif

[defragster]

Paul's changes from Mar 12 introduced this problem. Before, the register stuff was hard-coded. Now there is a member with the register addresses that needs to be initialized.

"20008000" is a garbage value - the TwoWire constructor hasn't run yet. Using that as register address causes the CPU to hang.

Good find tni.

Yes. "startup_late_hook()" runs after global variable initialization, it's already too late.

About startup_late_hook() - I am printing millis() time entry on setup() - I expected to see it push that entry time to higher millis() - I did not see that - seemed like the code was not running?

[mlu]

You could try to define the startup hook as extern "C" to avoid C++ name mangling.

extern "C" void startup_late_hook(void) {
  delayMicroseconds(100000);
  delayMicroseconds(100000);
  delayMicroseconds(100000);
  delayMicroseconds(100000);
}

[defragster]

@mlu: It takes enclosing braces to not get grief from the compiler about redefining the "C" object - startup_late_hook() does work to delay entry to setup - which may be useful in some cases. And apparently delayMicroseconds() is not safe for use when startup_early_hook() is called so early:

I:/Code/FORUM/amPrint/amPrint.ino:55:39: error: conflicting declaration of 'void startup_late_hook()' with 'C' linkage
 extern "C" void startup_late_hook(void)
                                       ^
I:/Code/FORUM/amPrint/amPrint.ino:7:13: note: previous declaration with 'C++' linkage

<edit> I had given that a try before moving on seeing that wouldn't help . . . but found this to work ...

extern "C"
{
  void startup_late_hook(void)
  {
    delayMicroseconds(400000);
  }
}
extern "C"
{
  void startup_early_hook(void)
  {
    delayMicroseconds(40000); // don't do this here!
  }
}

[PaulStoffregen]

Yikes, looks like the change to making Wire support multiple ports now make it susceptible to the C++ static initialization order fiasco. Didn't see that one coming (but probably should have).

For now, the "solution" is to avoid calling Wire.begin() from the AM2321 constructor. Just comment out line #141 in AM2331.cpp. Then call Wire.begin() in setup().

Long-term, we're going to need some sort of hack with static data in the Wire library so this case can be detected and handled. I'm putting this on my high priority bug list....

[tni]

Long-term, we're going to need some sort of hack with static data in the Wire library so this case can be detected and handled. I'm putting this on my high priority bug list....

Look at post #14. That enforces initialization. It's C++ standard-compliant and guaranteed to work.

Another possible solution is to make the TwoWire constructor constexpr, but that would also need to be done for all parent classes.

[sigi]

For now, the "solution" is to avoid calling Wire.begin() from the AM2321 constructor. Just comment out line #141 in AM2331.cpp. Then call Wire.begin() in setup().

Yep, already done that. Works perfect for me. But i understand also that this hurts the Arduino "Easy going, it just works" philosophy

[PaulStoffregen]

Yes. A future version will fix this.

Almost all libs call Wire.begin() from their own begin() method. This one is very unusual to call it from the constructor. That's why this bug was missed during beta testing.... we only tested about a dozen libs, none of them quite like AM2331.

Other boards which support more than one Wire with the same lib very likely suffer from this bug.

I will fix it in a future Teensyduino release.

[tni]

Pull request for the constexpr fix:
https://github.com/PaulStoffregen/cores/pull/232
https://github.com/PaulStoffregen/Wire/pull/7

Unfortunately, there is no way to keep the old "port" usage. It breaks constexpr.

EDIT:
For an explanation why this works:
https://akrzemi1.wordpress.com/2012/...nitialization/

[PaulStoffregen]

I've merged tni's pull requests and I'm starting a fresh round of Wire library testing today.