SPI.h: remove unused variable warnings

[pascallanger]

Small change to get rid of the annoying false positive unused warnings.

[lacklustrlabs]

Thumbs up for building with warnings enabled 👍

About those variables, are they used anywhere? Can they be outright deleted?

[pascallanger]

Yes they are used so they can't be deleted (otherwise I'll have proposed so).
Removing the warning is the only option unless some functions are written.

[stevstrong]

Wait: they are used but you have to mark them as "unused"?
Isn't this a contradiction?

[pascallanger]

It's directly used in SPI.cpp:
_spi1_this = (void*) this;
same for the others _spi2_this, _spi3_this
The warning is there because the declaration is done in the .h but used in the .cpp...
The ff is also declared in .h and used in .cpp.
I could rewrite the library differently but I thought you would prefer that I do not that. This is why I'm just clearing the warnings...

[stevstrong]

I think the solution for this warning should look different.
Those variables should be moved to the cpp file, or added to the class itself.

[pascallanger]

If you are open to that then I'll fix the root cause.

[pascallanger]

Ok done. I've moved the variables declaration where they should be.
No more warnings coming from this library.
Are you ok to merge it this way?

[stevstrong]

Now it looks fine to me, but I think it should be first tested if it still works as expected.
It was Victor who added these variables, so hes opinion is also asked.
Finally, it is Roger's decision whether to merge it or not.

[pascallanger]

That's why I was proposing the first option to just get rid of the warnings without touching the code... There was no testing needed and no brainer to merge.
Let's see if it gets through.

[victorpv]

The corrections look right to me. It was always my intention to move them within the class, but I forgot.
They are indeed needed for using the SPI DMA callback feature.

@stevstrong about the ff variable. Since the port can be configured in 16bit mode, I have been thinking for a while that we should perhaps declare that as ffff = 0xFFFF so it can be used to see FF repeatedly when in 16 bit mode. Otherwise we may be sending a random byte.

[stevstrong]

yepp, it was also once a concern of mine, as I reviewed your changes.
However, I don't know in which way are the added functions 16 bit mode compliant?

[victorpv]

Hold on this, Seems like you are just moving them to the cpp file but keeping them outside the class.
My initial intention was rather to move them within the class, and declare them as:

static SPIClass *_spi1_this;
static SPIClass *_spi2_this;

Let me test this out. I remember during development of the callback part I had some problem that prompted me to move them out of the class and declared as void*, but the problem was likely caused by something else and I should have moved them back to the class after resolving it, that's why I put a note on them.
I have a sketch that uses the callback functions, I'll compile and run it today in a board to confirm the callbacks work correctly in that case. If so, I think it would be preferable to have them be within the class even if static, because they are part of the SPI class.

[victorpv]

@stevstrong which added functions are you referring to? the ones I added a while back, to be able to use callbacks and return inmediately, they work as the ones before, only I split them in 2 parts, one part does the DMA setup that doesn't change between calls if the buffer doesn't change, and the other part just sets the count of bytes/in16 to send, and fires DMA, but as far as 16 bit compatibility they are like they used to be, can send both 8bit or 16bit depending on which mode the SPI peripheral is.
I'll check but I believe my test rig uses 16bit for the LCD part. If not, I'll change it. It's a sketch that uses SDFat with callbacks in one port, and SPI with callbacks and 16 bits for an LCD in the other port.

[victorpv]

Today I made the changes to move spi1_this etc to inside the class, not just to the cpp file, but to be part of the class. Also used 0xFFFF for the ff variable, and tested all with a display in spi2 and sdfat in spi1, the display using 16bit mode, and both using callbacks. The display does not ever send FFFF, since it doesn't read anything only writes. SDFat sends FF to read data, but uses only 8bit mode, so the only bit I have not tested is to send FFFF in 16bit mode, but can't think that I have any use case or example for that.
We could test sending it from one port and capturing with the other SPI port, that's the best I can think of right now to test that part.

[stevstrong]

I think it would be safe to have the variable "ff" 16 bit width.
Moreover, as @lacklustrlabs pointed out, it must not be class member, as it is invariant (constant), independent from other class parameters and number of classes declared in the application.
The other variables (spi class pointers) should be class members.

[victorpv]

I think the spi ones should be private and part of the class, because there is no reason anything else should use or modify them, they are for private use, so the fit the bill.
About FFFF, I agree we can leave it out, but if the intention is to make it available for other parts of the code, then perhaps should be in some core file with other constants such as PI.

[pascallanger]

I've tried to move static SPIClass *_spi1_this in the class but it then generated other warnings that I haven't been able to get rid off... This is why I left them in the cpp.

[victorpv]

Pascal I have a version with those as part of the class, I'll share it in a bit, I was trying so see if 0xFFFF was anywhere as part of the core that we could use it.
I only see it in macros and local variables.
So I'll leave ff as non-class static, and spi_this as class members.

[rogerclarkmelbourne]

Is this still a work-in-progress ?

It looks like you guys are still testing and amending this PR.

Or is it now stable, and if so how should it be tested, e.g. before and after ?

[lacklustrlabs]

Just checked to compare before and after, and the [-Wunused-variable] warnings are gone 👍
Looks good to me

[stevstrong]

Not good.
Static variable declaration should be removed from the function.

[victorpv]

Someway I overwrote my local copy where I modified it to work fine and without errors, so I just had to go and redo it again, and tested with SDFat and ILI0163 versions that use the callbacks with RTOS to switch tasks while waiting for the callback to be called. So confirmed 100% they work for SPI1 and SPI2. I do not have anything around using SPI3, but should work just as well for it.
These are my changes. Note I have changed the ff variable to be a const (so it's in flash) and to be 16bit wide, so works when the SPI port is in 16bit mode and a DMA transfer using it is done (this worked fine so far I believe, but more by chance since the DMA requires 16bit aligned address when doing 16bit transfers, anyway it takes just 1 byte of flash extra, but doesn't even show when I compile I guess because it was wasting 1 byte before due to alignments).

Please do this changes and test:
First remove the ff declaration from the functions.
In SPI.h, move the declarations down to the private: block:

private:

    static void (*_spi1_this);
    static void (*_spi2_this);
    #if BOARD_NR_SPI >= 3
    static void (*_spi3_this);
    #endif
    static const uint16_t ff;
    ...

Then in SPI.cpp, define them as class variables. I placed them right above the constructor:

const uint16_t SPIClass::ff = 0xFFFF;
void (*SPIClass::_spi1_this) = 0;
void (*SPIClass::_spi2_this) = 0;
#if BOARD_NR_SPI >= 3
void (*SPIClass::_spi3_this) = 0;
#endif

/*
 * Constructor
 */
...

I compiled in Sloeber, it works fine and doesn't give any warning.

[victorpv]

@pascallanger did you see my comment above? if you want to test that solution, it works fine for me and removes the warnings, at the samee time that keeps them within the class.

[rogerclarkmelbourne]

I think there is a reasonable chance this may be superseded.

There seems to be multiple ongoing investigations into SPI problems, so I'm not sure if I'll need to park this one until the dust settles ????

[stevstrong]

I think that by moving static uint8_t ff = 0XFF; outside the class (for example after the spi pointers) would be safe to merge, it solves the original issue.

[victorpv]

The change I suggested works fine, removes the warnings, and it keeps those variables inside the class and away from any other code trying to use a variable with the same names. I have not seen any update from the OP on whether he tested my suggestion, but I have tested it extensively.

@stevstrong can you test the changes I suggested I couple of posts above? just to doublecheck, since the work for me. In that case I will send a PR with the corrections.

[stevstrong]

I know I said before that the SPI class pointers should be class members, same as you (@victor) suggested, but now I am not so sure about that anymore. Because one instance should have only one pointer to himself, not three (for other instances, too) which happens if we have three pointers for each port. Same for current_settings[], it does not make much sense to have it for each instance in part, as the current class is selected by the index, and this should be global. Or am I missing something?

I think both ways (inside and outside of class) would work, but not sure about which way makes sense.

[victorpv]

If they are static in the class, there is only 1 copy of them, not one per instance, so being outside or being static inside, there is the same number of them. Being inside the class though keeps their scope local to the class, which I think makes the most sense.
CurrentSettings is a different story because there is one per instance.

I think the issue with the callbacks and currentSetttings is a different and separate problem, that will have to deal with in a different way.

[victorpv]

Now, if instead of making them static within the class, we made them normal class members, then there could be only 1 per instance, but then you can't do the proper callback form the core ISR driver. It can not call a member function that is not static. That's the whole reason of having the pointers.
The core ISR DMA calls a static member function, that funtion uses the spiX_this pointer as pointer to the instance, and using that call the instance ISR function.

We can rethink all that and find another way to do it, but the way it is, having the spiN_this pointers as statics outside or inside the class doesn't make any difference in the memory used, only 3 pointers for the class, no matter the number of instances, so I think we should apply that change, then we keep working on the subject of setModule and callbacks.

[rogerclarkmelbourne]

@victorpv

I forgot about C++ not being able to call a pointer from a class instance :-(

So I agree, we'd need a static array of pointers

[tpruvot]

made a new one in PR #653 which also fix the other warnings

[stevstrong]

Overruled by #653 .