Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix return type of ArduinoLEDMatrix::begin() #282

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

mast-eu
Copy link
Contributor

@mast-eu mast-eu commented Mar 16, 2024

While playing around with the LED matrix on my Arduino R4 WiFi I noticed that the function ArduinoLEDMatrix::begin() causes a compiler warning:
image
(This is what I observe after setting Preferences -> Compiler Warnings to All)

The reason appears to be very simple: ArduinoLEDMatrix::begin() is declared to return an int, but never returns anything. I think this is actually ok, since the function has nothing useful it could return.
Thus I send you this PR to change it's return type from int to void.

@CLAassistant
Copy link

CLAassistant commented Mar 16, 2024

CLA assistant check
All committers have signed the CLA.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Mar 16, 2024
@facchinm
Copy link
Member

Hi @mast-eu ,
I'd prefer to keep the int return and return 1 if the timer was correctly started, 0 otherwise to be coherent with the usual semantic of begin().
Would you mind amending the PR in this way?

@facchinm
Copy link
Member

Could supersede #225

@mast-eu
Copy link
Contributor Author

mast-eu commented Mar 25, 2024

Yes, I can amend the PR. But why do you want to keep int? Shouldn't bool be the better choice?
I'm looking for some examples of begin() in the code and came across

bool FspTimer::begin(timer_mode_t mode, uint8_t tp, uint8_t channel, uint32_t period_counts, uint32_t pulse_counts, timer_source_div_t sd, GPTimerCbk_f cbk /*= nullptr*/ , void *ctx /*= nullptr*/ ) {
or
bool PwmOut::begin() {

@facchinm
Copy link
Member

bool is fine too, I'd just avoid the signature change but there's no strong reason 🙂

@mast-eu mast-eu marked this pull request as draft March 28, 2024 23:10
@mast-eu mast-eu marked this pull request as ready for review March 30, 2024 16:47
@mast-eu
Copy link
Contributor Author

mast-eu commented Mar 30, 2024

@facchinm Please have a look. I've changed the implementation to provide a meaningful return value.

I preferred bool over int, because maybe it's just me, but with int I'd expect 0 to indicate that everything is ok and any non zero value to be an error code. While with bool it's exactly the other way.

@facchinm
Copy link
Member

facchinm commented Apr 2, 2024

Super, looks much better now! Merging

@facchinm facchinm merged commit 68ea03f into arduino:main Apr 2, 2024
7 checks passed
@mast-eu mast-eu deleted the compilerWarning branch April 2, 2024 11:34
@misaz
Copy link
Contributor

misaz commented May 11, 2024

I think this change was incorect because of using bool type. Data type of begin() is actually important because begin() of ArduinoLEDMatrix overrides begin() of ArduinoGraphics and ArduinoGraphics define begin as virtual int begin(); (https://github.com/arduino-libraries/ArduinoGraphics/blob/master/src/ArduinoGraphics.h#L41), so overriding it with method with bool output type naturally ends up with mismatch output type compiler error.

In file included from C:\Users\michal\AppData\Local\Temp\.arduinoIDE-unsaved2024411-121224-1myqqzw.l57b\TextWithArduinoGraphics\TextWithArduinoGraphics.ino:3:0:
C:\Users\michal\AppData\Local\Arduino15\packages\arduino\hardware\renesas_uno\1.1.0\libraries\Arduino_LED_Matrix\src/Arduino_LED_Matrix.h:172:10: error: conflicting return type specified for 'virtual bool ArduinoLEDMatrix::begin()'
     bool begin() {
          ^~~~~
In file included from C:\Users\michal\AppData\Local\Temp\.arduinoIDE-unsaved2024411-121224-1myqqzw.l57b\TextWithArduinoGraphics\TextWithArduinoGraphics.ino:2:0:
c:\Users\michal\Documents\Arduino\libraries\ArduinoGraphics\src/ArduinoGraphics.h:41:15: error:   overriding 'virtual int ArduinoGraphics::begin()'
   virtual int begin();
               ^~~~~

facchinm added a commit to facchinm/ArduinoCore-renesas that referenced this pull request May 13, 2024
delta-G pushed a commit to delta-G/ArduinoCore-renesas that referenced this pull request Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants