electro-smith / libDaisy

Hardware Library for the Daisy Audio Platform
https://www.electro-smith.com/daisy
MIT License
313 stars 131 forks source link

Remove deprecated volatile operators #480

Closed claudiocabral closed 2 years ago

claudiocabral commented 2 years ago

C++20 has deprecated some operators for volatile types (++, +=, and other things like that). This PR rewrites the couple of instances where that apperas in libDaisy.

stephenhensley commented 2 years ago

@claudiocabral thanks for the PR -- I was actually thinking about wanting to see what would have to change to support C++20.

Looks like pretty simple changes overall. Are you able to compile a full program with C++20, or are there more violations (in the HAL files, or elsewhere)?

brbrr commented 2 years ago

Tried it in my project, and I getting a bunch of other volatile violations from libdaisy/Drivers/CMSIS such as: error: compound assignment with 'volatile'-qualified left operand is deprecated [-Werror=volatile]

claudiocabral commented 2 years ago

@stephenhensley @brbrr I have a Daisy Patch, so only tested on that using cmake. I'll switch the flags in DaisyExamples and compile the whole thing to see what else I catch

claudiocabral commented 2 years ago

Turns out I was not compiling libDaisy with c++20, only the files I included in my project. like @brbrr said, there are tons of violations in Drivers/CMSIS. @stephenhensley These files are autogenerated, no? if not, I don't mind rewritting it all

stephenhensley commented 2 years ago

@brbrr I recall a similar experience from a while back.

Building just application code with C++20 should be fine with the fixes applied here, though.

I think trying to get all of the CMSIS/HAL stuff compiling with C++20 is a bit of an undertaking, and there might already be something underway by ARM and/or ST. So might be worth looking into that before editing all of those files.

If this let's you use C++20 for actual user code vs library code, then I think this is fine as is 😄

claudiocabral commented 2 years ago

@stephenhensley I've grepped for volatile in libDaisy headers in the src folder and these are the only occurences.

claudio@libDaisy $ rg volatile src/*.h src/**/*.h
src/util/ringbuffer.h
158:    volatile size_t read_ptr_;
159:    volatile size_t write_ptr_;

src/per/qspi.h
189: *  for the physical volatile memory.

src/dev/leddriver.h
281:    volatile int8_t current_driver_idx_;

src/dev/flash_IS25LP080D.h
98:#define WRITE_READ_PARAM_REG_CMD 0xC0     /**< volatile */
99:#define WRITE_NV_READ_PARAM_REG_CMD 0x65  /**< non-volatile */
100:#define EXT_WRITE_READ_PARAM_REG_CMD 0x63 /**< volatile */
102:#define WRITE_EXT_READ_PARAM_REG_CMD 0x83    /**< volatile */
103:#define WRITE_EXT_NV_READ_PARAM_REG_CMD 0x85 /**< non-volatile */
143:    /* Non volatile Configuration Register */

src/dev/flash_IS25LP064A.h
126:    /* Non volatile Configuration Register */

I suggest we merge it and address other C++20 issues as they come. Would you like me to rebase to squash it into a single better written commit?

stephenhensley commented 2 years ago

@claudiocabral I usually squash/commit PRs on this repo anyway. So no worries there.

And agreed. I'll merge this as is, and we can address further issues separately.

Thanks again for the contribution!