Simpler READ/WRITE macros?

[kaysievers]

A purely cosmetic issue. Could we change the current macro, or add additional ones? Like this:

#define TMC2130_READ(tdef, address, field) \
        FIELD_GET(tmc2130_readInt(tdef, address), field ## _MASK, field ## _SHIFT)
#define TMC2130_WRITE(tdef, address, field, value) \
        (tmc2130_writeInt(tdef, address, FIELD_SET(tmc2130_readInt(tdef, address), field ## _MASK, field ## _SHIFT, value)))

It turns the current long lines with repeated names:

TMC2130_FIELD_WRITE(d, TMC2130_COOLCONF, TMC2130_SGT_MASK, TMC2130_SGT_SHIFT, 15);
TMC2130_FIELD_WRITE(d, TMC2130_GCONF, TMC2130_EN_PWM_MODE_MASK, TMC2130_EN_PWM_MODE_SHIFT, 1);

Into something easier to read and less prone to errors while typing:

TMC2130_WRITE(d, TMC2130_COOLCONF, TMC2130_SGT, 15);
TMC2130_WRITE(d, TMC2130_GCONF, TMC2130_EN_PWM_MODE, 1);

https://github.com/versioduo/V2Stepper/blob/master/src/tmc/ic/TMC2130/TMC2130.h#L22 https://github.com/versioduo/V2Stepper/blob/master/src/Motor.cpp#L21

[trinamic-LH]

While this would work, im pretty sure that IDEs won't cope with this format - so trying to go to the defninition of TMC2130_SGT won't work as it is not defined and just a part of the full define name.

One thing that may work is to define the mask/shift values as a struct - something like this:

// The struct for fields
typedef struct {
    uint32_t mask;
    uint8_t shift;
    uint8_t regAddr;
} RegisterField;

// An example field definition
#define TMC2130_MY_FIELD ((RegisterField) { 0xFF000000, 24, 0x42 })

// Field access functions
int readField(RegisterField field);
void writeField(RegisterField field, int32_t value);

// Field access with struct macros
readField(TMC2130_MY_FIELD);
writeField(TMC2130_MY_FIELD, 42);

Atleast in theory this shouldn't cost much (if any) extra performance, is easy to read and will still allow a "Go to definition" IDE feature to work.

This would also be pretty similar to how we implemented register fields in python

[kaysievers]

This would also be pretty similar to how we implemented register fields in python

Something like this looks nice. It would combine the register and the field, which makes things a bit harder to get wrong.

I wasn't sure how much you wanted to change. The current 3 similar identifiers in every line just looked a bit much after a while :)

[trinamic-LH]

I agree, it does get redundant/overly verbose.

If that struct approach works out well I'd be happy to use that instead. Since the old mechanism is based on defines, keeping it around for backwards compatibility will be simple as well.

[trinamic-LH]

Small update: I tried out the struct defines combined with inlined read/write functions using said struct defines. This approach basically collapses to the same code as before after inlining and macro evaluation. Interestingly enough my conpiler managed to generate slightly cleverer mask/shfit assembly for the field writing when using the new approach.

I will get everything commited to the API_Revision branch sometime this week for you to try out

[trinamic-LH]

Struct-based register field access has been added in a46862b67bbbf66b9ec6deb6529ca5a6d96fe193.

[kaysievers]

Thanks a lot. It looks great.

I've updated everything and it appears to work all fine. It is nice that the registers and the fields can't mixed up in the wrong way now.

The only thing missing are the field definitions for the XDIRECT register: https://github.com/versioduo/V2Stepper/blob/master/src/Power.cpp#L42

[trinamic-LH]

That looks like a small mix-up on our end, should be a quick fix - on Monday ;)

Have a nice weekend :+1:

[trinamic-LH]

Fixed in 00948c94fed55073f597e8e38cd806613a25b315

[kaysievers]

Perfect. It works all fine now. Thanks a lot for all the changes.

[trinamic-LH]

No problem - thank you for all the feedback 👍

[trinamic-LH]

Since the *_FIELD macros are fully backwards compatible, we will be rolling this change out to all boards over time.

Closing this issue.