Skip to content

Pico - only one PWM pin is set #287

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.

Sign up for GitHub

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

Merged
DocMoebiuz merged 4 commits into from
Nov 30, 2023
Merged

Pico - only one PWM pin is set #287

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2432bf2
avoid double set of pin for Pico
elral Nov 26, 2023
ad8ef4f
keep old logic for AVR's
elral Nov 27, 2023
08558af
fixed missing ';'
elral Nov 27, 2023
2bee49e
use same set output logic for AVR's and Pico
elral Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CustomDevices
Open in desktop
Submodule CustomDevices updated from d7e3f3 to 6a2b28
16 changes: 14 additions & 2 deletions src/MF_Output/MFOutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,28 @@ MFOutput::MFOutput()
void MFOutput::attach(uint8_t pin)
{
_pin = pin;
#if !defined(ARDUINO_ARCH_RP2040)
#if defined(ARDUINO_ARCH_RP2040)
pinMode(_pin, OUTPUT_12MA);
digitalWrite(_pin, LOW)
#else
pinMode(_pin, OUTPUT);
set(_value);
analogWrite(pin, LOW);
#endif
}

void MFOutput::set(uint8_t value)
{
_value = value;
#if defined(ARDUINO_ARCH_RP2040)
Copy link
Collaborator

@DocMoebiuz DocMoebiuz Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be easier to read if we introduce a deviated method like

setRp2040 and then put the custom logic there instead of having everything inline

Copy link
Collaborator Author

@elral elral Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmhm, in this case the CommandMessenger.cpp must also be changed:

#if defined(ARDUINO_ARCH_RP2040)
    cmdMessenger.attach(kSetPin, Output::OnSetRP2040);
#else
    cmdMessenger.attach(kSetPin, Output::OnSet);
#endif

Due to the differnet pinMode initialization MFOutput.cpp would look like:

void MFOutput::attach(uint8_t pin)
{
    _pin   = pin;
    pinMode(_pin, OUTPUT);
    analogWrite(pin, LOW);
}

#if defined(ARDUINO_ARCH_RP2040)
void MFOutput::attachRP2040(uint8_t pin)
{
    _pin   = pin;
    pinMode(_pin, OUTPUT_12MA);
    digitalWrite(_pin, LOW);
}
#endif

And Output.cpp must be as:

#if defined(ARDUINO_ARCH_RP2040)
        outputs[outputsRegistered].attachRP2040(pin);
#else
        outputs[outputsRegistered].attach(pin);
#endif

Not sure if this increases readability, also as the CommandMessegner.cpp must also be changed (changes not as much bundled as before).

Copy link
Collaborator

@DocMoebiuz DocMoebiuz Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was thinking of a private local method only - nothing fancy

Copy link
Collaborator Author

@elral elral Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some more testing the logic from Pico to set outputs can also be used for AVR's.
So I wouldn't differ anymore between both. Just for setting pinMode() it has to be differ betwenn AVR and Pico.

The class MFOutput() is "only" needed for setting the pinMode and setting the powersave Mode.
I am wondering if we still need this class, both could also be done within Output.cpp like setting the output.

if (_value == 0xFF)
digitalWrite(_pin, HIGH);
else if (_value == 0x00)
digitalWrite(_pin, LOW);
else
analogWrite(_pin, _value);
#else
analogWrite(_pin, _value);
#endif
}

void MFOutput::powerSavingMode(bool state)
Expand Down
20 changes: 13 additions & 7 deletions src/MF_Output/Output.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ namespace Output
return;
outputs[outputsRegistered] = MFOutput();
outputs[outputsRegistered].attach(pin);
#if defined(ARDUINO_ARCH_RP2040)
pinMode(pin, OUTPUT_12MA);
analogWrite(pin, false);
#endif
outputsRegistered++;
#ifdef DEBUG2CMDMESSENGER
cmdMessenger.sendCmd(kDebug, F("Added output"));
Expand All @@ -52,11 +48,21 @@ namespace Output
// Read led state argument, interpret string as boolean
int pin = cmdMessenger.readInt16Arg();
int state = cmdMessenger.readInt16Arg();

// Set led
analogWrite(pin, state); // why does the UI sends the pin number and not the x.th output number like other devices?
// output[pin].set(state); // once this is changed uncomment this
#if defined(ARDUINO_ARCH_RP2040)
Copy link
Collaborator

@DocMoebiuz DocMoebiuz Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

if (state == 0xFF)
digitalWrite(pin, HIGH);
else if (state == 0x00)
digitalWrite(pin, LOW);
else
analogWrite(pin, state); // why does the UI sends the pin number and not the x.th output number like other devices?
// output[pin].set(state); // once this is changed uncomment this
#else
analogWrite(pin, state); // why does the UI sends the pin number and not the x.th output number like other devices?
// output[pin].set(state); // once this is changed uncomment this
#endif
}

void PowerSave(bool state)
{
for (uint8_t i = 0; i < outputsRegistered; ++i) {
Expand Down