Wait for PLL to lock, to avoid wrong data transfer on UART, in accord… - #160

Guy Francois,

Thank you very much for this Pull Request, and all the code that you have been developing.

Please can you tell me a little more about the "Wrong Data Transfer on UART".
I would really like to understand more about the specific problem that you have seen, that has prompted you to put in this PLL lock change.

I have exchanged email's with Bill on the topic of this Pull Request, and he is reluctant to change the code which has appeared to work for so many years. One of the good points about our Autopilot, is the manual control is almost immediate on power up. And on a more general point, we try not to put in any delay statements that just stop all the code. Bill's understanding of the current code is that by the time we send serial output, the PLL has locked. And that this has worked for a long time.

So it would be good for us to understand in more detail any testing that you have done, which definitely shows that the UART output is at the wrong speed because the PLL has not locked.

If we do make this change, then Bill feels it should go at the very end of initialisation, and that the serial output should come immediately afterwards. We will apparently need to test it very carefully.

Best wishes, Pete

Guy and Peter,
I think Guy is on the right track in response to Peter's summary of my thinking on this. I don't think it is necessary to worry about the pll locking for data coming in, we already have parsers for data coming in that handles bad data. For data going out, there is no point to send bad data, so the correct thing to do is to skip sending data if the pll is not locked. I think the pll will always be locked by the time the telemetry is ready to go out, so right now it makes no difference. However, in the future we may somehow change the code so it matters. So, just to be sure and to be "clean", I suggest we simply change the telemetry code to check for pll lock. Something like this:

#if (USE_MAVLINK == 1)
// Poll the MAVLink subsystem at 40hz
if ((udb_pulse_counter % (HEARTBEAT_HZ/40) == 0) && (OSCCONbits.LOCK == 1))
{
mavlink_output_40hz();
}
#endif // (USE_MAVLINK == 1)
#if (SERIAL_OUTPUT_FORMAT != SERIAL_NONE)
// Send telemetry updates at 8hz
if ((udb_pulse_counter % (HEARTBEAT_HZ/8) == 0) && (OSCCONbits.LOCK == 1))
{
// RobD flight_state_8hz();
telemetry_output_8hz();

That way there is no guessing whether or not enough time has gone by for the pll to lock, and there is no blocking anywhere if it is not, and no data goes out if the pll is not locked.

Best regards,
Bill

Hi Bill and Pete,
Working to explain why I suggest to wait for PLL to lock, I find an actual cause to explain what I observed on the telemetry output (high rate pulses during first 100's ms after power on, cf https://groups.google.com/forum/#!topic/uavdevboard/rQzL2itNdfE).
I agree with Bill that PLL will normaly be locked before sending data. Literature seems to say about 100 or 200 µs (1024 Fcy) to lock on.
I used the previous MP version (i.e. 4.1) where USART was initialized at the beginning of UDB init, except the baud rate which was defined when calling Init_Serial, at the end.
I do not remember why my code had 2 delays of 100 ms in the MPU init phase, but these delays added to the normal 60ms, generate sufficent time between USART init and Init_Serial to send 40 Hz Mavlink messages before that the correct baud rate (57600) will be set. Thus, these first messages are sent at 4 Mz, baud rate which could annoy the RFD868+ telemetry firmware.
In the current MP5, the baud rate is configured immediately after USART.
This issue could be fixed.
But the problem stays on the table : if we want to send message as soon as possible, when heartbeat is fired (for example sending telemetry without waiting calibration finished), the message sending is requested (on T6 interrupt) before the output driver will be configured. UDB is deaf!
This will have (probably???) no consequencies for the hardware.
But the software could think that it configures an equipment while this equipment receive nothing.

I will suggest to go ahead with Bill's idea and to create a boolean initilized to 0 at the beginning of the main init phase (matrixpilot_init() ) and set to 1 at the end. This boolean will be used to allow sending out messages.
I will write a report with more details and explanations and containing a review of all the UDB outputs that could be requested before the end of the init phase.
I am not familiar with HILSIM nor SILSIM, but this issue must be known by their users...

Best regards, gfm

Hi Guy,
I understand better now why you want the serial output to be guaranteed to be correct.
There are two conditions that must be satisfied in order to do that:

1. USART must be configured.
2. PLL must be locked.

In the latest version of MP, condition 1 is already taken care of, there is a global interrupt enable flag that gets turned on at the very end of initialization so that the code starts executing after everything is initialized:

void udb_init_irq(void)
{
SRbits.IPL = 0; // turn on all interrupt priorities
}

So, all we need to do is to satisfy the second condition. I propose we do that by checking the PLL locked bit in the code that generates telemetry, and skip sending data if it is not locked. I suggested that in my previous comment:

#if (USE_MAVLINK == 1)
// Poll the MAVLink subsystem at 40hz
if ((udb_pulse_counter % (HEARTBEAT_HZ/40) == 0) && (OSCCONbits.LOCK == 1))
{
mavlink_output_40hz();
}
#endif // (USE_MAVLINK == 1)
#if (SERIAL_OUTPUT_FORMAT != SERIAL_NONE)
// Send telemetry updates at 8hz
if ((udb_pulse_counter % (HEARTBEAT_HZ/8) == 0) && (OSCCONbits.LOCK == 1))
{
// RobD flight_state_8hz();
telemetry_output_8hz();

If you agree, I will create a pull request for you and Peter to look at that should make the telemetry output "bullet-proof".

Best regards,
Bill

Guy and Peter,
I am going to amend my most recent comment. I dig a little more reading of the interrupt documentation. It looks like it might be that the power up value of IPL is already zero, but I am not 100% sure of that. So I want to do a little more reading and thinking on this subject before suggesting a solution. Perhaps the best way to do it is to turn off the global interrupt enable as the first step on power up, and then turn it back on at the end of initialization. I will get back to you in a couple of days.
Best regards,
Bill

Hi Bill & Pete
When I test hypothesis on that issue, I force the observation of this sending to visualize with a logic analyser, that telemetry messages are sent to the USART during the matrixpilot init, meaning before setting the serial baud rate(s).
It is easy to raise this anomaly :
1- just add a delay greater than 200 ms in MPU6000_init16() ;
2- setup the udb_init_USART (as it is done for GPS uart) by calling mavlink_init at the beginning of main_init, without setting baud rate, so comment the line
// udb_serial_set_rate(MAVLINK_BAUD) ;
3 - Set baud rate at the end of the main_init ;
4- Send mavelink messages asap, not waiting for calib finished.

int matrixpilot_init(void)
{
gps_init(); // this sets function pointers so i'm calling it early for now
udb_init(); // configure clocks and enables global interrupts
mavlink_init();
// filesys_init(); // attempts to mount a file system
config_init(); // reads .ini files otherwise initialises with defaults
dcm_init();
#if (FLIGHT_PLAN_TYPE == FP_WAYPOINTS)
// init_waypoints();
#endif
servoMix_init();
servoPrepare_init();
init_states();
init_behavior();
udb_serial_set_rate(MAVLINK_BAUD);
telemetry_init();

This operation shows that message try to be sent during the init phase, before its end, so before setting the baud rate.
Best regards,
gfm

I think the question that has now arisen is whether the Interrupts are turned on or off when the CPU starts up. To partially answer that question, I have test commented out the re-enabling of interrupts like this:-

void udb_init_irq(void)
{
	//SRbits.IPL = 0; // turn on all interrupt priorities
}

I then ran the code in HILSIM. The Servos run fine, the Plane flys correctly in FBW mode, and the telemetry (SERIAL_UDB_EXTRA in my case) all comes out of the UDB5 correctly.

So udb_init_irq() is currently not really enabling the interrupts. They appear to be enabled all ready. And as a side note, I see that there is a separate and more appropriate Global Interrupt Flag, that can be set or unset to stop all interrupts, without changing the settings of SRbit.IPL. That said, Setting SRBits.IPL = 7, will stop all the main external interrupts from working.

Best wishes, Pete

Hi Bill & Pete,
I have tested the hypothesis of blocking the interrupt by setting SRBits.IPL = 7.
It seems to work.
But, as Bill is worry about, we have to understand quite deeply what this IPL means.
The literature is not very clear about IPL :-(
Does it masks only the external interrupt priorities?
What about the internal interrupt, like DMA's one?
Perhaps it stops the external interrupt int3, stopping heartbeat in 200 Hz mode. But does it stop also the timer T1 interrupt, in case of heartbeat 40 Hz mode?
What append if the A/D converter is running, firing DMA interrupts, without heartbeat resetting the sample_count counter?
As my spiritual father said, some many years ago "Toute modif est source de panne", something like "Any change is failure sources", I suggest to be close to the first Bill's idea to check some adequate indicator before sending out UART2 serial data.
I believe that there is no problem with GPS, nor I2C.
I continue to write the promised report,
Best regards,
gfm