possible bug in ocompare_config

XBee & XBee Gateway XBee Programamble (S2C & 900 HP)
1

I installed the latest Xbee Programmable SDK (build 1.1.4) and I noticed something interesting. In ocompare.c in function ocompare_config() the init_timeout is never used other than to test that it’s less than max_timeout, and TPMCNT is always set to 0. In other words that parameter is more orless ignored. The document does not reflect this. It could be that some older functionality was deprecated.

I have forked this function and made my own copy, because I don’t want to lose everything it does. For me, the main thing is that if I set up a periodic output compare I want TPMMOD to be settable. Currently it only sets it to 0, which essentially means unbounded and free-running (16 bits worth, at whatever prescaler it decides is appropriate).

I think for the next version of the library Digi ought to consider making these modules a little more robust, and in particular, add the ability to configure the timer to be more precisely periodic. If user contribution is welcome I’ll donate the code to do so.

–Chris

2

Hello Chris,

Thank you very much for your feedback. It is a known issue that will be fixed in next release. However, I would like to know how you expect these modules to behave. Currently, periodicity is made “manually” by setting the next timeout every time the user callback is reached. User feedback is always welcome, if you want to share your function changes and thoughts it will be appreciated and taken into consideration.

Let us know how it works for you and if you think that the API needs more flexibility. Best regards,

Sebastián.-

P/S:
Current ocompare_config() is:

int ocompare_config(xpin_t pin, bool_t enable, uint32_t max_timeout, uint32_t init_timeout, uint8_t behavior)
{
	tpm_t tpm = xpin_get_tpm(pin);
	uint8_t channel = xpin_get_tpm_ch(pin);

	if (!xpin_has_tpmch(pin))
		return -EINVAL;

	if (init_timeout > max_timeout)
		return -EINVAL;
	
	if (behavior != OCOMPARE_TOGGLE &&
		behavior != OCOMPARE_CLEAR &&
		behavior != OCOMPARE_SET)
		return -EINVAL;

	tpm_clock_gating(tpm, 1);
	REG_TPMSC(tpm) = 0;
	
	/* Prescaler for ocompare won't change in ocompare_set_timeout calls */
	if (tpm_set_prescaler(tpm, max_timeout) < 0)
		return -EOVERFLOW;
	
	REG_TPMCSC(tpm, channel) = TPM1C0SC_MS0A_MASK | (behavior << 2);
	REG_TPMCV(tpm, channel) += tpm_compute_modulo(REG_TPMSC(tpm), init_timeout); /* Set timeout */
	REG_TPMMOD(tpm) = 0; /* free-running counter */
	REG_TPMCNT(tpm) = 0;
	ocompare_irq_enable(pin, enable);
	
	return 0;
}
3

Hi there,

I expected it to be free running from the time I start it. I would do something like this:

int ocompare_config_ext(xpin_t pin, bool_t enable, uint32_t period, uint8_t behavior)
{
        tpm_t tpm = xpin_get_tpm(pin);
        uint8_t channel = xpin_get_tpm_ch(pin);

        if (!xpin_has_tpmch(pin))
                return -EINVAL;
        
        if (behavior != OCOMPARE_TOGGLE &&
                behavior != OCOMPARE_CLEAR &&
                behavior != OCOMPARE_SET)
                return -EINVAL;

        tpm_clock_gating(tpm, 1);
        REG_TPMSC(tpm) = 0;
        
        /* Prescaler for ocompare won't change in ocompare_set_timeout calls */
        if (tpm_set_prescaler(tpm, max_timeout) < 0)
                return -EOVERFLOW;
        
        REG_TPMCSC(tpm, channel) = TPM1C0SC_MS0A_MASK | (behavior << 2);
        REG_TPMCV(tpm, channel) += tpm_compute_modulo(REG_TPMSC(tpm), init_timeout); /* Set timeout */
        REG_TPMMOD(tpm) = period; /* NO LONGER FREE RUNNING! */
        REG_TPMCNT(tpm) = 0;
        ocompare_irq_enable(pin, enable);
        
        return 0;
}

I got rid of the initial/starting point, but if somebody really needed that, you could use it to set TPMCNT (you’d set it to max_timeout - init_timeout). I don’t see much purpose to that, though, especially since it currently doesn’t work.

The advantage of this arrangement is that you can put out pulses (let’s say square waves) at a harware-precise interval. Then, in your IRQ, all you’re responsible for is setting the next pin state, and you have the entire period to do so. In other words, you completely eliminate jitter due to interrupt latency, and you have as long as the entire period to service the interrupt.

In my case, the only thing the isr does is set_behavior() (I think that’s what it was called - it’s not in front of me right now) to set the next state.

If you’re worried about breaking backward compatibility, another way to do this would be to add a new function similar to set_timeout, and name it something like set_timeout_periodic that sets TPMMOD and TPMCNT. That’s essentially what I did to get to a solution.

By the way, my problems still aren’t over. Even when I set the behavior to TOGGLE, I’m not getting very square waves this way. I suspect the problem is the way the config function chooses its dividers. I’ll figure this out later in the week and get back to you with results.

4

Hello Chris,

There is a little error in your code, where:


        REG_TPMCV(tpm, channel) += tpm_compute_modulo(REG_TPMSC(tpm), init_timeout); /* Set timeout */
        REG_TPMMOD(tpm) = period; /* NO LONGER FREE RUNNING! */

Should be:


        REG_TPMCV(tpm, channel) += tpm_compute_modulo(REG_TPMSC(tpm), init_timeout); /* Set timeout */
        REG_TPMMOD(tpm) =  REG_TPMCV(tpm, channel); /* NO LONGER FREE RUNNING! */

Because these registers count clocks, not microseconds by themselves (tpm_compute_modulo() function calculates how many pulses is ‘init_timeout’ according to it’s prescaler and clock-source.

Another thought, the implementation (even with the fixes) is not able to reach 10 uS, it’s not documentated anywhere (it will be in next release). I tested it and the lowest I could reach was 80 uS.

If the timeout will not change, it’s OK to set the TPMxMOD to the same value as TPMxCxV so it’ll automatically reset the counter. It was made that way in the very first implementation (not released) but it turned out to be not good for an example like the burst_wave in waveform_generation. The problem with this is that both TPMxMOD and TPMxCxV registers have a latching mechanism that writes the value after counter reaches (TPMxMOD - 1) (topic 16.3.3 TPM Counter Modulo Registers in Microcontroller’s Reference Manual ) and this causes that the module loses synchronicity. You may set CLKS bytes in TPMxSC to 00 so it writes the modulo value immediately, but when doing so the output-compare module is disabled and the pin state is driven by the I/O ports (you’d see a glitch). Also, any thing done in the ISR must be very fast, so you may want to remove all sanity-checks like:


if (!xpin_has_tpmch(pin))
	return -EINVAL;
5

thanks for catching that. Yeah, I was typing that from memory … when I wrote it, i actually did it correctly.

I don’t understand the clock architecture enough to know what all these clocks are, but it seems like if the clock source was the bus clock that would be 48 MHz and it should be possible for the output compare to work at a very high rate. Unless for some reason the hardware actually is working fine and I’m just missing interrupts.

I will try setting it to TOGGLE and make the ISR just do nothing, and see what comes out.

6

Our API works with two clock sources: Internal reference clock (31.25 kHz) and bus clock which is configurable at config.xml, component System. So the minimum BusClock is 24 MHz (SysClock = 48 MHz and BusClockDiv = 2). Bear in mind that the callback that you see in main.c is not the ISR, it’s called by the ISR after clearing the flag.

7

I tried a couple of experiments.

First, I set the behavior to TOGGLE, and programmed it for 10 uS. I modified my interrupt handler to just return without doing anything. What I got out was a perfect square wave with a period of 20 uS (50% duty cycle, 10 uS per half cycle). perfect.

Then, I changed the default behavior to CLEAR, and I modified my interrupt handler to set it to CLEAR or SET every other time it’s called. Then, I turned off the watchdog timer and (just for testing purposes) stopped calling the xbee tick. The radio’s not joined, so all UARTs should be quiet.

When I did this, I get pulses that, instead of 10 uS, are generally either 40 uS or 50 uS.

So this is telling me that the whole problem here is interrupt latency. But why? I’m shocked it’s that long. The interrupt stack frame is 5 bytes; your actual IRQ is 3 instructions until it calls my function; and my function is 18 instructions plus the call to set_mode.

Oh. The call to set_mode is pretty crazy, actually. It’s using a bunch of functions to go find it. I think my next step needs to be to profile the actual amount of time spent in the interrupt. I just did a quick calculation, though, and at 48 MHz, 10 microseconds is 480 cycles. Instruction lengths vary but assuming an average of 4 cycles, that’s only 120 cycles. I can see why we’re blowing it up. That ISR needs to be more like a few dozen cycles. That doesn’t necessarily mean it can’t be done in C, but all the HAL type stuff that locates the right tpm from the pin, etc. is going to have to be greatly streamlined.

I bet I can do this by just writing my own tpm-set_mode that caches the pointer to the correct TPMCSC register.

8

Hello Chris,

First of all, thank you very much for your feedback and clear explanations on the tests and needs.
Clearly for your case our API will not be fast enough and you may have to modify the functions. I think that the following table may help you:

[b]S2B		S2C		TPMxCx		PIN_PWMCH()[/b]
XPIN_4 		XPIN_5		TPM2C1SC	PWMCH(4)
XPIN_6	 	XPIN_7		TPM3C5SC	PWMCH(11)
XPIN_12		XPIN_25		TPM3C0SC	PWMCH(6)
XPIN_13		XPIN_26		TPM2C0SC	PWMCH(3)
XPIN_17		XPIN_30		TPM2C2SC	PWMCH(5)
XPIN_20 	XPIN_33		TPM1C0SC	PWMCH(0)

The PIN_PWMCH() is a bit-mask that were used by early implementation of PWM to address the TPM registers with a switch. There were three macro defined to use that:

#define PWM_CFG_PS_MASK		0x07
#define XPIN_GET_PWM_CH(pin)	(((pin) >> PWMCH_SH) & 0x0f)
#define XPIN_IS_PWMABLE(pin)	(XPIN_GET_PWM_CH(pin) != 0xf)

They were used in switch-case as follows:

	channel = XPIN_GET_PWM_CH(pin);
		
	switch (channel) {
	case 0:		TPM1C0SC = x;
				break;
	case 3:		TPM2C0SC = x;
				break;
	case 4:		TPM2C1SC = x;
				break;
	case 5:		TPM2C2SC = x;
				break;
	case 6:		TPM3C0SC = x;
				break;
	case 11:	TPM3C5SC = x;
				break;
	default:	return -EINVAL;
	}

Finally, if you want to code your own ISR, comment the original implementation macro (e.g.: ocompare_irq_handler(2,1) for XPIN_4 in S2B) and implement

void tpm2ch1_isr(void)

in your code (remember to clear the flag in TPM2C1SC_CH1F!)

…or put the #pragma INLINE in top of the user_callback in main.c

I hope it helps. Let me know if you have any doubt or need more information on how these modules are implemented. Best regards,

9

Finally, if you want to code your own ISR, comment the original implementation macro (e.g.: ocompare_irq_handler(2,1) for XPIN_4 in S2B) and implement

Is there an easy way to do that? I didn’t really want to modify files that are part of the SDK, but if I modify the ones that are brought into my project by config.xml won’t they get overwritten if I change the configuration? I was thinking the only reasonable looking way to do this might be to replace the vector in the RAM copy of the vector table with my own…

10

config.xml can modify only xbee_config.h with #includess and #defines and main.c with code-snippets. If you modify xbee_config.h every time you save config.xml the contents are regenerated and you loose the changes. However, if you want to manually define some macros you can use custom.h which is included by xbee_config.h.

The interrupt vector table is placed at irq_vectors.c in folder “sources/cpu” but it’s a const so you can’t change it at runtime.

Maybe the easiest way will be to exclude the file ocompare.c from build in your project (Right click on file->Resource Configurations->Exclude from build…), copy the file to Sources (in the same folder as main.c) and modify it locally, leaving unaffected the original.

Best regards,