Pico - only one PWM pin is set#287
Conversation
DocMoebiuz
left a comment
DocMoebiuz
left a comment
There was a problem hiding this comment.
see my comments - have you thought about it?
src/MF_Output/MFOutput.cpp
Outdated
| void MFOutput::set(uint8_t value) | ||
| { | ||
| _value = value; | ||
| #if defined(ARDUINO_ARCH_RP2040) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
i was thinking of a private local method only - nothing fancy
There was a problem hiding this comment.
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.
src/MF_Output/Output.cpp
Outdated
| // 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) |
|
Firmware for this pull request: |
|
Firmware for this pull request: |
DocMoebiuz
changed the title
DocMoebiuz
changed the title
DocMoebiuz
changed the title
2 participants
Description of changes
Further evaluation of issue #283 showed that PR #285 didn't solve the issue.
Pico is limited to one bank of 16 PWM timer starting at Pin 0. If using a pin > 15 also as PWM output, these pins get doubled.
E.g. using pin 2 as PWM output and unsing pin 18 as PWM output each change on pin 2 or pin 18 will also set the other one.
If one of these pins are used as a digital output everything is fine.
Within the code for the outputs
analogWrite()is always used toset an output. Doing it this way each pin is used as PWM output for the Pico and the above problem occurs. The only way to solve it is to use only the first 16 pins as PWM output. This change will be done in a separate PR to adapt theraspberrypi_pico.board.jsonby setting"isPWM": falsefor pins > 15. So for pins > 15 the value for setting the pin will either be0or255.Within the FW it has to check if the value for each pin is
0or255. In this casedigitalWrite()is used instead ofanalogWrite()which avoids setting the output as PWM.This change is only done for the Pico, for AVR's it is kept as it is to not raise another potential problem (which I can not see, just to be absolutely sure).
Fixes #283