How do I get this line of code to work?

Go To Last Post
14 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi

 

I have a question that I think relates more to general coding, and I hope it makes sense with the info below

 

Alternative 1 works fine.

Alternative 2 does not, how come? How should I write to set the LSB of CTRLC directly, without the temp-detour?

 

Thanks Thomas

 

// Alternative 1

TC0_t *temp;

temp = &pwm_cfg.tc;

temp->CTRLC = temp->CTRLC || 1;

 

// Alternative 2

(TC0_t *)(&pwm_cfg.tc)->CTRLC = (TC0_t *)(&pwm_cfg.tc)->CTRLC || 1;

 

//Definitions

struct pwm_config pwm_cfg;    

struct pwm_config {
    void *tc;
    enum pwm_channel_t channel;
    enum tc_cc_channel_mask_enable_t cc_mask;
    enum pwm_clk_sel clk_sel;
    uint16_t period;
};

 

typedef struct TC0_struct
{
    register8_t CTRLA;  /* Control  Register A */
    register8_t CTRLB;  /* Control Register B */
    register8_t CTRLC;  /* Control register C */
    register8_t CTRLD;  /* Control Register D */
    register8_t CTRLE;  /* Control Register E */
    register8_t reserved_0x05;
    register8_t INTCTRLA;  /* Interrupt Control Register A */
    register8_t INTCTRLB;  /* Interrupt Control Register B */
    register8_t CTRLFCLR;  /* Control Register F Clear */
    register8_t CTRLFSET;  /* Control Register F Set */
    register8_t CTRLGCLR;  /* Control Register G Clear */
    register8_t CTRLGSET;  /* Control Register G Set */
    register8_t INTFLAGS;  /* Interrupt Flag Register */
    register8_t reserved_0x0D;
    register8_t reserved_0x0E;
    register8_t TEMP;  /* Temporary Register For 16-bit Access */
    register8_t reserved_0x10;
    register8_t reserved_0x11;
    register8_t reserved_0x12;
    register8_t reserved_0x13;
    register8_t reserved_0x14;
    register8_t reserved_0x15;
    register8_t reserved_0x16;
    register8_t reserved_0x17;
    register8_t reserved_0x18;
    register8_t reserved_0x19;
    register8_t reserved_0x1A;
    register8_t reserved_0x1B;
    register8_t reserved_0x1C;
    register8_t reserved_0x1D;
    register8_t reserved_0x1E;
    register8_t reserved_0x1F;
    _WORDREGISTER(CNT);  /* Count */
    register8_t reserved_0x22;
    register8_t reserved_0x23;
    register8_t reserved_0x24;
    register8_t reserved_0x25;
    _WORDREGISTER(PER);  /* Period */
    _WORDREGISTER(CCA);  /* Compare or Capture A */
    _WORDREGISTER(CCB);  /* Compare or Capture B */
    _WORDREGISTER(CCC);  /* Compare or Capture C */
    _WORDREGISTER(CCD);  /* Compare or Capture D */
    register8_t reserved_0x30;
    register8_t reserved_0x31;
    register8_t reserved_0x32;
    register8_t reserved_0x33;
    register8_t reserved_0x34;
    register8_t reserved_0x35;
    _WORDREGISTER(PERBUF);  /* Period Buffer */
    _WORDREGISTER(CCABUF);  /* Compare Or Capture A Buffer */
    _WORDREGISTER(CCBBUF);  /* Compare Or Capture B Buffer */
    _WORDREGISTER(CCCBUF);  /* Compare Or Capture C Buffer */
    _WORDREGISTER(CCDBUF);  /* Compare Or Capture D Buffer */
} TC0_t;

This topic has a solution.

Last Edited: Mon. Sep 12, 2016 - 11:37 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thomasx wrote:
Alternative 1 works fine.

How do you check for that?

Cause this does not look right:

// Alternative 1
TC0_t *temp;
temp = &pwm_cfg.tc;

//Definitions
struct pwm_config pwm_cfg;
struct pwm_config {
    void *tc;
    ...
};

First, why void instead of TC0_t?

More importantly, what you want is the value of pwm_cfg.tc (the address of a TC0_t), not the address of the pwm_cfg.tc field itself.

Thus you should have instead:

temp = (TC0_t*)pwm_cfg.tc;

Right?

ɴᴇᴛɪᴢᴇᴎ

Last Edited: Mon. Sep 12, 2016 - 09:14 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

netizen wrote:

thomasx wrote:

Alternative 1 works fine.

 

How do you check for that?

Well, I know it works,  becuase I can see the resulting signal changing state on my oscilloscope in Alternative 1, just as expected, while Alternative 2 won't compile. ;)
 

netizen wrote:

Cause this does not look right:

// Alternative 1
TC0_t *temp;
temp = &pwm_cfg.tc;

//Definitions
struct pwm_config pwm_cfg;
struct pwm_config {
    void *tc;
    ...
};

First, why void instead of TC0_t?

This is part of the code generated by ASF for the PWM service (Pulse width modulation). Why Atmel/ASF chose to define it as void I don't know, but I guess the have a reason.

Here's an example of what it looks like when pwm_cfg->tc gets passed on to a function call, it is received as a volatile void pointer, and then converted to (TC0_t *) when used.

static inline void tc_write_period(volatile void *tc, uint16_t per_value)
{
    ((TC0_t *)tc)->PER = per_value;

}

Why they did it like this would be interesting to know, I assume. But for me, I am more interested to learn how to access CTRLC directly, withoug going via an extra variable or a function call, which also works, I tried that too.

What I want to do in practice is to set the LSB of CTRLC to 1, in order to force the PWM output to High when the PWM is not running (ie. stopped).
 

 

 

netizen wrote:

More importantly, what you want is the value of pwm_cfg.tc (the address of a TC0_t), not the address of the pwm_cfg.tc field itself.

 

Thus you should have instead:

temp = (TC0_t*)pwm_cfg.tc;

Right?

Well, pwm_cfg.tc already is a TC0_t pointer, as is temp, so no need to convert, I guess. Right? 
Also, if I don't use the & to get the adress, temp will be assigned the entire struct as its own entity, a copy, which would result in something I don't really know what it would be.
On the other hand, I guess writing

temp = pwg_cfg->tc;

should work fine, which makes me wonder why using the & actually worked. As you are implying, should it not return the adress where the pointer is stored, rather than the pointer.

Hmmm, I need to catch up on this, haven't done any C-programming for like 20 years ;)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

STOP STOP STOP!

I just noticed I had missed to comment out one important code line.

 

This does NOT! work

temp = &pwm_cfg.tc;

 

This DOES work

temp = pwm_cfg.tc;

My mistake. So it means I ended up in the correct reasoning in the end, I assume :)
 

Ok, so now that's sorted :)    Thank you for pointing it out!

Back to the original question :)

How to get the one-liner to work.

Last Edited: Mon. Sep 12, 2016 - 09:52 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thomasx wrote:
Well, pwm_cfg.tc already is a TC0_t pointer, as is temp, so no need to convert, I guess. Right?

Wrong: pwm_cfg.tc is a void* pointer. That's why it has to be cast to TC0_t*, just like in your tc_write_period() example:

static inline void tc_write_period(volatile void *tc, uint16_t per_value) {
    ((TC0_t *)tc)->PER = per_value;
}

 

thomasx wrote:
Also, if I don't use the & to get the adress, temp will be assigned the entire struct as its own entity, a copy, which would result in something I don't really know what it would be.

Wrong again: temp will be assigned the value of pwm_cfg.tc, which is exactly what you want. This value happens to be the address of a TC0_t.

 

thomasx wrote:
On the other hand, I guess writing temp = pwg_cfg->tc; should work fine

Wrong yet again: pwm_cfg is not a pointer thus you can't use the -> operator.

 

thomasx wrote:
Hmmm, I need to catch up on this, haven't done any C-programming for like 20 years ;)

You most definitely need to re-work on your understanding of struct and pointers: you have them all messed up at the moment.

 

thomasx wrote:
Well, I know it works, becuase I can see the resulting signal changing state on my oscilloscope in Alternative 1, just as expected

I'm not debating what you see on your scope: I'm debating why. I'm afraid you are falling victim to a confirmation bias: you get the right output for the wrong reason, thus your conclusion is flawed.

ɴᴇᴛɪᴢᴇᴎ

Last Edited: Mon. Sep 12, 2016 - 10:08 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thomasx wrote:
I just noticed I had missed to comment out one important code line.

This does NOT! work

temp = &pwm_cfg.tc;

This DOES work

temp = pwm_cfg.tc;

It also raises a compiler warning because you should cast it to TC0_t*, as mentioned earlier. :-)

 

thomasx wrote:
Ok, so now that's sorted :) Thank you for pointing it out!

Back to the original question :)

How to get the one-liner to work.

Now that you're back on track, I'll let you give it a shot. :-)

ɴᴇᴛɪᴢᴇᴎ

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Figured it out :)

 

This is what it should look like, of course :)

 

((TC0_t *)pwm_cfg.tc)->CTRLC = ((TC0_t *)pwm_cfg.tc)->CTRLC || 1;
 
And it works.

Thanks for helping my brain getting started :)

The way I wrote it first tried to make a TC0_t out of CTRLC, and on top of that had the & that shouldn't be there.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

netizen wrote:

 

It also raises a compiler warning because you should cast it to TC0_t*, as mentioned earlier. :-)

I agree that it should, but it actually doesn't. I guess it is default set to not warn for missing type casts, or something like that.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Excellent. Now go and conquer the world! ;-)

ɴᴇᴛɪᴢᴇᴎ

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thomasx wrote:

Figured it out :)

 

This is what it should look like, of course :)

 

((TC0_t *)pwm_cfg.tc)->CTRLC = ((TC0_t *)pwm_cfg.tc)->CTRLC || 1;
 
And it works.

Thanks for helping my brain getting started :)

The way I wrote it first tried to make a TC0_t out of CTRLC, and on top of that had the & that shouldn't be there.

I tried so many different ways, funny I didn't try that one. Its the one that makes the most sense :)

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

netizen wrote:

 

Wrong yet again: pwm_cfg is not a pointer thus you can't use the -> operator.

 

Aha, so that's the difference in using -> versus .  as in pwm_cfg.tc

Of course.
Things are beginning to fall into place :) 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In post #1,

How should I write to set the LSB of CTRLC directly

 

In post #8,

((TC0_t *)pwm_cfg.tc)->CTRLC = ((TC0_t *)pwm_cfg.tc)->CTRLC || 1;

 

Why are you using the logical OR (||) instead of the bit-wise OR (|)?

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 Oh, that was just a typ-o that I now see slipped in here too ;)

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

thomasx wrote:
How should I write to set the LSB of CTRLC directly, ...

 

|= 1; ?

 

Or maybe, to add a bit of documentation

 

|= (1<<CPMA);

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.