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/hal/gen3 pinmode #1777

Merged
merged 5 commits into from May 16, 2019

Conversation

@YutingYou
Copy link
Contributor

commented May 15, 2019

Problem

  1. User LED is configured to OUTPUT mode when initializing.
  2. Pin mode is changed to default mode after calling analogWrite() and digitalWrite()

Reference issue:
#1772

Steps to Test

  1. Flash example
  2. If D7 is OUTPUT mode, the device will enter PANIC mode
  3. Use a multimeter to measure OUTPUT value from D4 pin, it should be high now

Example App

void setup(void) {
    SPARK_ASSERT(getPinMode(D7) == PIN_MODE_NONE);

    pinMode(D4, OUTPUT);
    analogWrite(D4, 128);
    digitalWrite(D4, 1);
}

References

[CH32946]


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

  • [bugfix] [gen 3] pinMode fixes, D7 was initialized as OUTPUT mode, analogWrite() and digitalWrite() were changing pinMode back to default after use #1777

@YutingYou YutingYou requested a review from avtolstoy May 15, 2019

@avtolstoy avtolstoy added this to the 1.2.0-rc.1 milestone May 15, 2019

@@ -126,7 +126,6 @@ void LED_Init(Led_TypeDef Led) {
return;
}

HAL_Pin_Mode(HAL_Leds[Led].pin, OUTPUT);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

Does this affect RGB LED pins in any way?

This comment has been minimized.

Copy link
@YutingYou

YutingYou May 15, 2019

Author Contributor

We use PWM for RGB LED, PWM will overwrite GPIO configuration, so RGB LEDs work correctly.
But it will affect USER LED.

LED_Init() is only called once to initialize all LEDs at boot, if we don't configure it output, USER LED should not work.

HAL_Core_Led_Mirror_Pin() -> HAL_Led_Init() -> LED_Init()
Set_System() -> LED_Init()

Gen2 should have this issue too, I found Gen2 also configures USER LED as output in LED_Init(), should we leave the output configuration?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

👍

I see no reason why we should by default configure it as output. We should double check the current behavior for Gen 2 though.

@YutingYou YutingYou force-pushed the fix/hal/gen3-pinmode branch 2 times, most recently from 335fb1a to 20c5e09 May 16, 2019

@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Change the fix for user LED issue.
Fixes PWM channel duplicate assignment.

Need rereview, thanks. cc: @avtolstoy

@avtolstoy avtolstoy force-pushed the fix/hal/gen3-pinmode branch from 20c5e09 to 027742d May 16, 2019

@avtolstoy avtolstoy merged commit 5068605 into develop May 16, 2019

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@avtolstoy avtolstoy deleted the fix/hal/gen3-pinmode branch May 16, 2019

@technobly

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I see the PWM channels were changed in this PR for D5-D7, but I don't see why in the problem/solution. Does this affect the documentation? @avtolstoy @YutingYou

@technobly technobly referenced this pull request May 17, 2019
5 of 6 tasks complete
@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@technobly This PR is the fix for issue #1772, it includes pin mode issue and duplicated PWM channel.

Does this affect the documentation?

It doesn't. It's for the Nordic driver, we only care about PWM instance in Particle PWM chapter.
image

@technobly

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@YutingYou ok thanks, I see now that the change looks to align all Gen 3 platforms to use the same channels for PWM on D5-D7. I noticed you didn't list the D7 output above, yet PWM is available on D7.

@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@technobly D7 works with RGB LED. It should be this part in Particle Docs:

Group 3: Pins D2, D3, A4, and A5.
Group 2: Pins A0, A1, A2, and A3.
Group 1: Pins D4, D5, D6, and D8.
Group 0: Pin D7 and the RGB LED. This must use the default resolution of 8 bits (0-255) and frequency of 500 Hz.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.