IronHeartConsulting / IBPV2

International Beacon Program - V2.0
6 stars 2 forks source link

At init, sometimes controller seems out of synch #2

Closed K6TD closed 9 years ago

K6TD commented 9 years ago

On the first boot, if the init completes just as it's time to start the transmission, the controller seems out of synch. CW ID isn't right, long dashes don't seem to be long enough, and band changes are erratic.

Might be that the time of day jumps into the middle of a state.

wa5znu commented 9 years ago

It is supposed to suppress transmission until it has sent an id. On Jun 18, 2015 8:54 AM, "Kevin Rowett" notifications@github.com wrote:

On the first boot, if the init completes just as it's time to start the transmission, the controller seems out of synch. CW ID isn't right, long dashes don't seem to be long enough, and band changes are erratic.

Might be that the time of day jumps into the middle of a state.

— Reply to this email directly or view it on GitHub https://github.com/IronHeartConsulting/IBPV2/issues/2.

K6TD commented 9 years ago

Haven't heard this re-occur.

K6TD commented 9 years ago

This may be caused by the time wheel not starting on the right minute. wall_ticks is defined as 'volatile byte'. In the GPS-DO routine, it's used in a computation. The entire computation may get truncated to a byte operation, thereby overflowing when minutes goes above 04. No direct evidence yet.

K6TD commented 9 years ago

ah...here is the code from TinyGPS::crack_datetime:

void TinyGPS::crack_datetime(int year, byte month, byte day, byte hour, byte minute, byte second, byte hundredths, unsigned long age) {

This looks like 'minute' could end up overflowing in GPS_do

GeoNomad commented 9 years ago

gps.crack_datetime(&year, &month, &day, &hour, &minute, &second, &hundredths, &age); if (age == TinyGPS::GPS_INVALID_AGE) ... oops else ... OK

Looks like you are not checking for invalid age ...

wa5znu commented 9 years ago

The invalid-age thing does look like it needs to be be fixed; thank you @GeoNomad . GPS is not needed very often, but we do need to start listening to it, then but we do need to know when we get a valid parse from it.

wa5znu commented 9 years ago

@K6TD checking to see if code looks like it will overflow. This looks like the computation:

wall_ticks = (wall_ticks+1) % (3*60);

K6TD commented 9 years ago

That code should be fine, as wall ticks range is 0-179, less than 8 bits, or 256. The place that worried me was this: wall_ticks = ((minute * 60) + second) % (3*60); In boolean gps_discipline_clock(long tries) {

minute is a byte, with a range of 0-59. (I hope, NOT 1-60). It's cast as a byte. The compiler might limit the computation minute * 60 to a byte then also. At a value of 5, the result will be 300, and it might wrap, truncate, etc, resulting in the wrong wall_ticks value. An assertion check might be 0<= wall_ticks < 180.

wa5znu commented 9 years ago

I tried it on Intel. I haven't tried it on Arduino yet. https://gist.github.com/wa5znu/ffb603f96bbdf209c0ba makes it seem like the overflow isn't happening there. Intermediate results in C++ are supposed not supposed to be limited, but the assignment may be confusing. We can either run this test code or we can change the code in gps_discipline_click to introduce intermediate variables and see if that makes the problem not re-occur. My bet is more on GPS invalid check, but maybe it's both.

K6TD commented 9 years ago

I expect Intel processor/compilers to get it right. It's the Arduino/Atmel compiler that has byte wide machine instructions, and the compiler might use them.

K6TD commented 9 years ago

Overflow isn't happening. See this GIST. https://gist.github.com/e14a4b8ba5f93bbbbde3.git The only thing I can think of is not checking for a fix.

wa5znu commented 9 years ago

I ran a test too, showing overflow is not happening. https://gist.github.com/wa5znu/e3480cdb7cdc0ffa13bb

K6TD commented 9 years ago

Will apply Peter's suggestion of testing for invalid age test.

K6TD commented 9 years ago

scanning the log file, didn't find any occurrences of clock done: 1 - doesn't look like it ever got thru the retries loop with a good packet.

wa5znu commented 9 years ago

The setup() routines goes through or else it wouldn't have the time at all. So perhaps it just needs to be more aggressive about tries, without consuming all the tick time.

K6TD commented 9 years ago

Fixed the GPS DO code such that it now gets synch every time, an with a fix age of usually 3ms. Added a bit of delay in the gps do routine if the serial character buffer is empty. GPS just got turned back on. It may be some time before a character shows up.