Array pointer in struct and AtmelStudio: a bug or my ignorance?

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

Hi, I am writing a circular buffer where originally its structure was the classic one (I guess), in the rbuffer.c lib:
 

struct ring_buff
{
  uint8_t buff[RB_SIZE];
  rb_sz_t head;
  rb_sz_t tail;
  rb_sz_t count;
};

The problem is that in the serial.c I instantiate 2 buffers, for TX and RX. But one of them may need a rather smaller size. And I might want to use the ring buffer for other communication code, which could include very long buffers or jsut few chars.

 

So I thought:

 

struct ring_buff
{
  uint8_t** buff;
  rb_sz_t head;
  rb_sz_t tail;
  rb_sz_t count;
};

Because when I dereference buff I need to get back a pointer of where my (serial, for example) buffer is located.

So I declare:

uint8_t tx_buff[SER_RB_SIZE];
uint8_t rx_buff[SER_RB_SIZE];


struct ring_buff* serial_tx_buff;
struct ring_buff* serial_rx_buff;

and I initialize pointers in this way:

(*(uint8_t**)serial_tx_buff->buff) = tx_buff;
(*(uint8_t**)serial_rx_buff->buff) = rx_buff;

 

The test echo code, if with the original code was working, now does gobbledygook. So, to cut down the problem, here the first issue.

Here you see the initial statues of the array and their pointers:

 

 

When initializing, the poiner of tx get correcty stored in the tx struct. But also on the rx one (???).

 

 

Then, when the RX buff gets assigned, for some reason its value is stored in the location (of data!) of the tx buff.

 

 

If I solve this, I may think where I am wrong with the read and write. But I believe there is something very wrong with what I am doing.

 

Is it even possible at all? Any alternative?
 

This topic has a solution.
Last Edited: Tue. Jul 30, 2019 - 11:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You appear to have one more level of indirection than you need?
.
Anyway I can't help thinking this whole thing is crying out for C++ and templates ;-)

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

How did you created instances for ring buff pointers?

struct ring_buff* serial_tx_buff;
struct ring_buff* serial_rx_buff;

As it's on a global scale it's set to zero by default, but as you can see its value in watch 1, both are still pointing to the address 0 - that usually means invalid - null pointer dereference.

Computers don't make errors - What they do they do on purpose.

Last Edited: Tue. Jul 30, 2019 - 09:22 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You only declare pointers to your structs - you never actually create a structure! What you want is:
struct ring_buff serial_tx_buff;

In your structure, you don’t want ** buff, just *buff

Remember, a pointer is a variable with a memory address - it needs to point to something.

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

Kartman wrote:
You only declare pointers to your structs - you never actually create a structure! What you want is: struct ring_buff serial_tx_buff; In your structure, you don’t want ** buff, just *buff Remember, a pointer is a variable with a memory address - it needs to point to something.

 

KIIV_cz wrote:

How did you created instances for ring buff pointers?

 

Thank you so much. I was CONVICED was exactly the same, as long as I take care how accessing it (with . or -> and how to pass them in functions).
When I think about that now, a char* c; does not allocate anything! True!

clawson wrote:
You appear to have one more level of indirection than you need? . Anyway I can't help thinking this whole thing is crying out for C++ and templates ;-)

 

Well now that I solved it after 3 days of hitting my head here and there, everything is not writable in a very normal way.
When I dig down into the rabbit hole, as english speakers would say, I started to imagine one more pointer making more sense. I was lost in the declaration.

Last Edited: Tue. Jul 30, 2019 - 11:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As an aside, i usually typedef the struct so i’m not forever typing struct. Maybe others can comment whether this is a good strategy?

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

Kartman wrote:

As an aside, i usually typedef the struct so i’m not forever typing struct. Maybe others can comment whether this is a good strategy?

Good or not, that's what I do as well.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Kartman wrote:
As an aside, i usually typedef the struct so i’m not forever typing struct. Maybe others can comment whether this is a good strategy?

I was doing it a lot, but then I started to think if I really know why, or if is just to make the code looking "cool". So I started to define a bit of a coding style.

So when see a name of a srtuct, I prefer to just call it "struct name" instead of "name_str", as clearly says is a struct and is not a typing error. Also, I redo a typedef when really there is a need to adapt the types, and usually redefining standard types for abstraction (I think this is related to some standard C rules behind as well, as is a type definition, not a struct or name definition). Unless there are pointers involved that might avoid the typing of & and * all over the place, then it makes more sense.

 

Aside from that, I also very rarely use camelCases versus snake_cases, and prefer readability versus compact renaming.

Basically, I try to apply the naming conventions used in the Linux kernel and documentation. I see it paradoxically clearer to read than a simpler firmware like FreeRTOS (which does the opposite, with the hungarian notation).

I believe for standard reasons is better to use spaces, but for lazyness I use tabs. Until I found a editor that reformat tabs to spaces automagically.

Last Edited: Wed. Jul 31, 2019 - 08:07 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

joeymorin wrote:

Good or not, that's what I do as well.

In the coding standard I follow (for C not C++) this is actually mandated. The point at which typedef'ing really comes into it's own is for really complex function pointers. If you have to type it as a cast every time you need to assign a value it can become horrendous!
thexeno wrote:
as clearly says is a struct and is not a typing error
Isn't the fact that something is a struct a bit of a give away whether the name contains "struct", "_str" or whatever?

typedef struct {
    char name[20];
    uint8_t age_years;
    float height_m;
} record_t;

record_t people[3];

...
    strcpy(people[2].name, "Dave Smith");
    people[2].age_years = 37;
    people[2].height_m = 1.84;

...

void output_data(record_t * rec) {
    printf("Name: %s\n", rec->name);
    printf("Age: %u\n", rec->age_years);
    printf("Height: %.2f\n", rec->height_m);
}

    output_data(&people[1]);

All the '.' and '->' are a sure giveaway that structs are involved ;-)

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

clawson wrote:

 

All the '.' and '->' are a sure giveaway that structs are involved ;-)

 

I was reading this, and checking against the real source code (and I liked that)
https://www.kernel.org/doc/html/v4.10/process/coding-style.html

 

 

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

Yeah I quite like the Linux kernel style but I'm going to disagree with his arguments about struct typedefs. 

 

But coding standards, like indentation styles (which can also be part of a coding standard in fact) are almost like politics or religion - everyone has their own strong views and not everyone agrees with everyone else.

 

So usually you just have to agree to differ ;-)

 

BTW regarding your initial question. I did mention C++ templates. Now I have to admit I'm a neophyte when it comes to templates but I tried:

#include <avr/io.h>

template<typename T, int bytes>
struct ring_buff{
		T buff[bytes];
		T head;
		T tail;
		T size;
}; 

ring_buff<uint8_t, 20> uart_buff;
ring_buff<uint16_t, 30> adc_buff;

int main(void)
{
	uart_buff.buff[18] = 0x55;
	adc_buff.buff[23] = 0xAA;
    while (1)
    {
    }
}

and it seems to have worked:

	uart_buff.buff[18] = 0x55;
  8a:	85 e5       	ldi	r24, 0x55	; 85
  8c:	80 93 54 01 	sts	0x0154, r24	; 0x800154 <uart_buff+0x12>
	adc_buff.buff[23] = 0xAA;
  90:	8a ea       	ldi	r24, 0xAA	; 170
  92:	90 e0       	ldi	r25, 0x00	; 0
  94:	90 93 2f 01 	sts	0x012F, r25	; 0x80012f <_edata+0x2f>
  98:	80 93 2e 01 	sts	0x012E, r24	; 0x80012e <_edata+0x2e>

The .map file reveals:

 .bss.adc_buff  0x00800100       0x42 main.o
                0x00800100                adc_buff
 .bss.uart_buff
                0x00800142       0x17 main.o
                0x00800142                uart_buff

so the uart_buff is 23 bytes (20 for the buffer and 3 for the housekeeping) while the adc_buff is 66 bytes. 60 for the buffer (30 elements, each 2 bytes) and three 16 bit elements for the house-keeping.

 

I've probably done this wrong and C++ officiandos (of which I am not one!) will likely have comments about this. But it is true that templates in C++ do make this kind of thing easy to do.

 

(and yes I know I didn't need to make the house keeping elements the same type as the buffer but I was envisaging where you might want a wider type for the house-keeping should the array ever exceed 256 elements - it probably really wants two types passed in - one for the type of buffer and one to control the width of the indices)

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

I'm a c++ beginner (or 1 step above), here is a simple buffer from a class template-

https://godbolt.org/z/Xlqmvm

 

notice my coding style :) - not Linux approved. I 'invented' that when starting to use c++ (I don't think you will find that style listed in any ide editor). I wanted a way to see function names without having to mentally parse it from a sea of syntax (::).

 

All the function names, enum names, etc., stay 'separated' on the left so they can easily be seen/found. My github code mostly has this style (cv007), but it is not necessarily used for code that changes a lot as it does take a little extra effort. I like it for code that is 'stable'- drivers, common code. When you want to know what the Usart class can do, just open the hpp or cpp file with that name, browse up and down the left column looking at the function names which stand out and are easy to see. It kind of ends up as my only documentation for functions available (plus any comments there when needed), and seems to work well for my simple needs.

 

I suspect I am the only one with this style, but there are lots of weird people in the world so who knows.

Last Edited: Wed. Jul 31, 2019 - 11:19 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

curtvm wrote:
I suspect I am the only one with this style,
While this works fine if you work alone - as soon as you start to share with other programmers it may be a problem as it makes the code "difficult to read" for them (well it did for me anyway!)

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

I hate this style. I'm keeping track about features in C++ and I didn't even noticed that there is possible to define functions like this (but it looks more like lambdas).

Anyway, it's hard to read it for experienced C++ guy, it'll be impossible to understand by beginers.

Good code isn't about "hey, look what can I do" but about readability and quick understanding it by someone else if it's needed. And cryptic method definitions aren't really helping it at all. I couldn't even see that method names at first, because I'm not expecting them to be there. They are invisible to me.

 

Computers don't make errors - What they do they do on purpose.

Last Edited: Thu. Aug 1, 2019 - 10:09 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

>as soon as you start to share with other programmers it may be a problem as it makes the code "difficult to read"

 

Its mostly is used for 'read-only' code, where the main goal is to easily find what functions are available and at the same time find the source.

 

Try to find where in each of these, the function where the baud is being set-

https://github.com/cv007/PIC32MM...

https://github.com/torvalds/linu...

 

not really a fair comparison, but just points out that finding functions in source code can be difficult, and my 'solution' is to get the 'keywords' isolated into the left column and simply scrolling a page with your eyes looking at one side of the page, it seems easier to locate functions even when you are not sure exactly what you are looking for. Its maybe like a spreadsheet, with the keywords in column 1 and the code starts to the right of it in column 3. My example above has 'separators' //==== between functions, but that was version 1 of my style which I find unnecessary.

 

Like I said, probably a little strange/weird. Although it could be the start of something, just like eating a candy bar with a knife and fork.

 

and here is a 'Linux' style of my earlier buffer code (or as close as I can, have not read the whole style guide)-

https://godbolt.org/z/JtUBRl

and a c version (byte buffers only)-

https://godbolt.org/z/ES5GvW

 

Last Edited: Thu. Aug 1, 2019 - 11:18 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

curtvm wrote:
Try to find where in each of these, the function where the baud is being set-
I just typed Ctrl-F,"baud" in both - was that cheating?

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

>I just typed Ctrl-F,"baud" in both - was that cheating?

 

No, that's what we all do.

There are plenty of 'baud' to find, but what function do I need to call to set the baud, or which function sets the baud. Coming up with a find of 20 instances of 'baud' in my search tells me where the word is located, but now I have to backtrack to see what function its located in (and if this is the function I'm looking for). With what I have, I just quickly scan for functions (scroll the page) and I see 'baud_set' which is probably what I wanted- now I know what to call to set the baud, and I can see the source while I'm there. If that function calls another, it can also be quickly found.

 

Not trying to sell anything, or change how anyone else formats their code. Just showing a style I like to use.

 

>and I didn't even noticed that there is possible to define functions like this

Then you learned something new about how the compiler treats whitespace.

 

>I couldn't even see that method names at first, because I'm not expecting them to be there. 

But now you know where they will always be. All by themselves in the left side of your display.

Last Edited: Thu. Aug 1, 2019 - 12:40 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are comparing apples and oranges. I know you said:

curtvm wrote:
not really a fair comparison,
that's very true.

 

What you are actually comparing is some nicely structured code versus the usual dog's dinner from Atmel. A fairer comparison would be:

            auto Uart::
rx_errclr   () -> void
            {
            setbit(m_uartx_base + UXSTA, 1<<OERR);
            clrbit(m_uartx_base + UXSTA, 1<<OERR);
            }

//=============================================================================
            auto Uart::
rx_empty    () -> bool
            {
            return not anybit(m_uartx_base + UXSTA, 1<<URXDA);
            }

//uxbrg
//=============================================================================
            auto Uart::
baud_set    (uint32_t v) -> void
            {
            v = v < 110 ? 110 : v;        //minimum 110
            v = v > 921600 ? 921600 : v;  //maximum 921600
            m_uartx_baud = v;             //store
            //always use highspeed, simpler and works fine @ 24Mhz/921600
            //max clock and lowest baud also fits in uxbrg in highspeed
            v = baud_clk() / 4 / v;
            hispeed(true);
            //if v is 0 (clock too slow for desired baud),
            //just set to maximum baud for this clock
            val(m_uartx_base + UXBRG, v ? v-1 : 0);
            }

//called by clk_sel(), on()
//=============================================================================
            auto Uart::
baud_set    () -> void
            {
            //if baud not set, set it to 115200
            baud_set(m_uartx_baud ? m_uartx_baud : 115200);
            }

versus the more traditionally laid out:

void Uart::rx_errclr()
{
    setbit(m_uartx_base + UXSTA, 1<<OERR);
    clrbit(m_uartx_base + UXSTA, 1<<OERR);
}

//=============================================================================
bool Uart::rx_empty()
{
    return not anybit(m_uartx_base + UXSTA, 1<<URXDA);
}

//uxbrg
//=============================================================================
void Uart::baud_set(uint32_t v)
{
    v = v < 110 ? 110 : v;        //minimum 110
    v = v > 921600 ? 921600 : v;  //maximum 921600
    m_uartx_baud = v;             //store
    //always use highspeed, simpler and works fine @ 24Mhz/921600
    //max clock and lowest baud also fits in uxbrg in highspeed
    v = baud_clk() / 4 / v;
    hispeed(true);
    //if v is 0 (clock too slow for desired baud),
    //just set to maximum baud for this clock
    val(m_uartx_base + UXBRG, v ? v-1 : 0);
}

//called by clk_sel(), on()
//=============================================================================
Uart::baud_set()
{
    //if baud not set, set it to 115200
    baud_set(m_uartx_baud ? m_uartx_baud : 115200);
}

I'm a traditionalist - I know which I find quicker/easier to read.

 

BTW I undid the "auto ... -> rety_type" thing too - I have no idea how the auto based one is supposed to make the code any easier to read/maintain ??

 

Every C/C++ programmer on earth currently expects a function interface to be:

ret_type fn(in_type parms...) {

not sure what's gained by making this:

auto fn(in_type parms...) -> ret_ytpe {

??

 

(sometimes I think all this stuff they add in C++11, C++14, C++17, C++20 is just to see how far they can push obfuscation !)

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

>and I didn't even noticed that there is possible to define functions like this

Then you learned something new about how the compiler treats whitespace.

It wasn't about whitespaces...  I've seen "-> type" only for lambdas. So it looks so weird to see    auto something() -> bool { ... }. Well, it's more like pascal..

And as I'm used to read c++ code, I just don't expect names on the left. I rather use something like this:

 

class App
{
public:
	virtual void      update() = 0;
	virtual void        draw() = 0;
	virtual void        draw(ofRectangle const& area) = 0;
	virtual bool   isVisible() { return true; }
};

But of course, it kinda bloats if you have more complex types (but there is always using/typedef)

Computers don't make errors - What they do they do on purpose.

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

>I have no idea how the auto based one is supposed to make the code any easier to read/maintain ??

 

It eliminates having to specify the class name in the return value if the return value is from the class. Its not necessary, but there are times its nice to have, so to make it less of a mix I just always use it so everything is doing the same thing. One less :: to look at.

//hpp
struct Some{
        enum SOME { A, B };
        static SOME get();
        static SOME another();
};
//cpp
Some::SOME Some::get()
{
        return A;
}
Some::SOME Some::another()
{
        return B;
}

//hpp
struct Some {

            enum
SOME        {
            ATHING,
            BTHING
            };

            static auto
get         () -> SOME;

            static auto
another     () -> SOME;

};
//cpp
            auto Some::
get         () -> SOME
            {
            return ATHING;
            }

            auto Some::
another     () -> SOME
            {
            return BTHING;
            }

I guess my main goal is prevent having to visually parse function names out of its surrounding text. 

Last Edited: Thu. Aug 1, 2019 - 02:16 PM