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

Bootloader and Device OS use same HAL driver, Implement mirror button… #1590

Merged
merged 26 commits into from Mar 13, 2019

Conversation

@YutingYou
Copy link
Contributor

commented Nov 7, 2018

Problem

  1. Bootloader uses Nordic driver for button and led while Device OS uses Particle HAL driver. In this PR, both bootloader and Deivce OS will use Particle HAL driver.

  2. Implement mirror button and mirror led.

Note:

Bootloader increases to 32808 Bytes

   text    data     bss     dec     hex filename
  32808     812    6856   40476    9e1c ../build/target/bootloader/platform-14-lto/bootloader.elf

Steps to Test

1. Mirror Button Test

  1. Build and flash bootloader application
  2. Build and flash below example app
  3. Press button or mirror button to enter listening mode and exit listening mode
  4. Observe the console log, it outputs click counter and debounce time.
  5. Enter safe mode and DFU mode in the bootloader.
  6. Use mirror button D2 to repeat step 5

2. Mirror LED Test

  1. Build and flash bootloader application
  2. Build and flash below example app
  3. Use three LED and connect them to A3, A4, A5 pin
  4. Observe LED state in Device OS and bootloader

3. Mirror LED unit test

Test application path: /user/tests/wiring/no_fixture/led.cpp
You should change A7 pin to A3, because A7 doesn't exit on Xenon.

Example App

 #include "application.h"

SYSTEM_MODE(MANUAL);
Serial1LogHandler   logHandler(115200, LOG_LEVEL_ALL);

static volatile int counter = 0;

void setup() {
    RGB.mirrorTo(A3, A4, A5, false, true);
    // RGB.mirrorDisable(true);

    System.buttonMirror(D2, FALLING, true);

    System.on(button_click, [](system_event_t ev, int clicks) -> void {
        counter++;
    });
}

void loop() {
    Log.info("counter: %d", counter);
    delay(1000);
}

References

[CH20601]
[CH20613]
[CH19813]
[CH28529]

Completeness

  • [Enhancement] [Gen 3] Adds button and RGB LED mirroring support #1590

@YutingYou YutingYou requested a review from avtolstoy Nov 7, 2018

@@ -61,6 +61,7 @@ PinMode HAL_Get_Pin_Mode(pin_t pin);
void HAL_GPIO_Write(pin_t pin, uint8_t value);
int32_t HAL_GPIO_Read(pin_t pin);
uint32_t HAL_Pulse_In(pin_t pin, uint16_t value);
void HAL_GPIO_Toggle(uint16_t pin);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

Why not HAL_GPIO_Write(pin, !HAL_GIO_Read(pin))?

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Fixed

@@ -7,6 +7,6 @@ TARGET_TYPE = a
BUILD_PATH_EXT = $(BUILD_TARGET_PLATFORM)

# the only true dependency is Wiring, the others are transitive dependencies
DEPENDENCIES = wiring hal services system platform communication
DEPENDENCIES = wiring hal services system platform communication dynalib

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

Is this needed?

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Fixed

}

void Reset_System(void) {
__DSB();

SysTick_Disable();

sd_nvic_DisableIRQ(RTC1_IRQn);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

PWM needs to be deinited here too preferably.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Fixed


nrf_pwm_enable(NRF_PWM0);
uint32_t pwm_max = Get_RGB_LED_Max_Value();
HAL_PWM_Write_Ext(HAL_Leds[led].pin, HAL_Leds[led].is_inverted ? (pwm_max - value) : value);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

This will still work even if HAL_Core_Led_Mirror_Pin_Disable() was previously called, because there is no longer an is_active flag and runtime configuration is not cleared, so the version check will pass.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Fixed

uint8_t padding[14];
uint8_t version; // Struct version
uint16_t pin;
uint8_t is_inverted; // If LED is "inverted", active state is 0, instead of 1

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

For future extension I'd prefer if this was a bitmask.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Fixed

BUTTON_Init(BUTTON1_MIRROR, BUTTON_MODE_EXTI);

if (pin == HAL_Buttons[BUTTON1].pin) {
LOG(WARN, "Pin %d shares the same EXTI as SETUP/MODE button", pin);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 7, 2018

Member

This log line is confusing on nrf52840 devices. The original check on stm32f2xx was to prevent the user from using GPIOA7 and GPIOB7 pins, because EXTI interrupt is shared between the same pin numbers in different ports.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Nov 12, 2018

Author Contributor

Good to know, removed.

@avtolstoy avtolstoy added do not merge and removed needs review labels Nov 7, 2018

@YutingYou YutingYou force-pushed the feature/hal/mirror-button-led branch from ded25a2 to e11f10f Nov 12, 2018

@YutingYou YutingYou force-pushed the feature/hal/mirror-button-led branch from e11f10f to 3279a37 Jan 18, 2019

@@ -42,20 +43,6 @@ typedef enum
#define LED_RGB LED3_LED4_LED2
#define LED_USER LED1

// FIXME: These should be pulled from platform_config.h

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Jan 18, 2019

Member

The unit tests seem to be broken. Let's leave these ifndefs here with default values in addition to platform_config.h and create an empty platform_config.h for GCC platform in platform/MCU/gcc/inc.

This comment has been minimized.

Copy link
@YutingYou

YutingYou Jan 20, 2019

Author Contributor

Fixed

@avtolstoy avtolstoy removed the needs review label Jan 23, 2019

@avtolstoy avtolstoy referenced this pull request Jan 23, 2019
4 of 6 tasks complete

@avtolstoy avtolstoy force-pushed the feature/hal/mirror-button-led branch from 080381b to 837750e Jan 31, 2019

@avtolstoy
Copy link
Member

left a comment

There is a difference in brightness between the on-board LED and the mirror LED for some unknown reason when using a common anode LED (inverted = true).

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

8b64110 also fixes a 'red led stays slightly on' issue with common anode configuration (inverted = true).

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

I've also noticed that we are always reinitializing PWM on every value change, which is bound to cause glitches. Why are we not simply updating the duty cycles within nrf_pwm_sequence_t and reconfiguring only when necessary e.g. when changing frequency?

@YutingYou YutingYou force-pushed the feature/hal/mirror-button-led branch from 8b64110 to ad2fb74 Feb 13, 2019

@YutingYou

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Fixes PWM glitches caused by reinitialize PWM module when changing value. Reinitialization only happens when changing frequency or configuring a new PWM pin in a same PWM group.

before fix:
image

after fix:
image

This fix cannot pass unit test in user/tests/wiring/no_fixture/pwm.cpp, because of below limitation:

Listed here are the main features of one PWM module:
Change of polarity, duty-cycle, and base frequency possibly on every PWM period

It means we have to wait for a period then the duty-cycle will update. test(PWM_09_LowFrequencyAnalogWriteOnPinResultsInCorrectPulseWidth) will fail after testing 4-bit resolution and start to test 12-bit resolution, the error is big between these two resolution.

A dummy read of pulseIn() is needed after analogWrite(), should we add it to unit test?

@YutingYou YutingYou referenced this pull request Feb 13, 2019
2 of 6 tasks complete
@technobly

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@avtolstoy @YutingYou any updates on this PR? I will add the changes noted for no_fixture/pwm.cpp in a separate PR.

technobly added a commit that referenced this pull request Mar 11, 2019

@YutingYou YutingYou force-pushed the feature/hal/mirror-button-led branch from 7a03982 to e45c9bc Mar 12, 2019

@avtolstoy avtolstoy changed the base branch from mesh-develop to develop Mar 12, 2019

Eugene and others added 22 commits Nov 12, 2018

@avtolstoy avtolstoy force-pushed the feature/hal/mirror-button-led branch from 49ff46d to 55d4da9 Mar 13, 2019

@avtolstoy avtolstoy merged commit 4f1b37a into develop Mar 13, 2019

2 checks passed

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

@avtolstoy avtolstoy deleted the feature/hal/mirror-button-led branch Mar 13, 2019

@technobly technobly added this to the 1.1.0-rc.1 milestone Mar 13, 2019

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.