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

[RDY] Driver to control the external power output #242

Merged
merged 1 commit into from Oct 10, 2020

Conversation

megamind4089
Copy link
Contributor

@megamind4089 megamind4089 commented Oct 7, 2020

This PR adds support to control the external power output from controllers like nice!nano, nRFMicro etc

I have implemented based on my understanding of Pete suggestion on this feature.
I may also have overdone some parts.

Testing done:

  • Tested by enabling and disabling the ext_power from application and verified
  • Verified the application does not crash with boards that does not have ext_power support
    Note:
    I did not test this in nice!nano since I don't have the boards. Will get help from others once the behavior PR is up

Next Steps:

  • Create a behavior PR to control enable/disable ext_power

Copy link
Contributor

@petejohanson petejohanson left a comment

A few early thoughts. Thanks!

app/dts/bindings/zmk,ext-power-nicenano.yaml Outdated Show resolved Hide resolved
app/dts/bindings/zmk,ext-power-generic.yaml Show resolved Hide resolved
app/include/drivers/ext_power.h Outdated Show resolved Hide resolved
@megamind4089 megamind4089 force-pushed the feature/ext-power branch 3 times, most recently from 9dca81c to 2abe9f6 Compare Oct 8, 2020
Copy link
Contributor

@petejohanson petejohanson left a comment

The driver part of this looks really good. Just a few comments. Thanks!

app/dts/bindings/zmk,ext-power.yaml Outdated Show resolved Hide resolved
app/src/ext_power_generic.c Outdated Show resolved Hide resolved
app/src/ext_power_generic.c Outdated Show resolved Hide resolved
app/src/ext_power_generic.c Outdated Show resolved Hide resolved
@megamind4089 megamind4089 force-pushed the feature/ext-power branch 4 times, most recently from 7e846ce to 3f12c68 Compare Oct 9, 2020
@megamind4089 megamind4089 changed the title [WIP] Driver to control the external power output [ RDY] Driver to control the external power output Oct 9, 2020
@@ -31,6 +31,7 @@ target_sources(app PRIVATE src/sensors.c)
target_sources_ifdef(CONFIG_ZMK_DISPLAY app PRIVATE src/display.c)
target_sources(app PRIVATE src/event_manager.c)
target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/ble_unpair_combo.c)
target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/ext_power_generic.c)
Copy link
Contributor Author

@megamind4089 megamind4089 Oct 9, 2020

Choose a reason for hiding this comment

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

Can I add a Kconfig to control this driver ?

Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

Yes, I would say so. Definitely shouldn't be tied to the ZMK_BLE Kconfig setting.

@megamind4089 megamind4089 force-pushed the feature/ext-power branch 2 times, most recently from 14a29d4 to 2544744 Compare Oct 9, 2020
@megamind4089 megamind4089 changed the title [ RDY] Driver to control the external power output [RDY] Driver to control the external power output Oct 9, 2020
@petejohanson petejohanson added core Core functionality/behavior of ZMK enhancement New feature or request labels Oct 9, 2020
Copy link
Contributor

@petejohanson petejohanson left a comment

Couple minor comments. This is really close to merging. Thanks.

@@ -31,6 +31,7 @@ target_sources(app PRIVATE src/sensors.c)
target_sources_ifdef(CONFIG_ZMK_DISPLAY app PRIVATE src/display.c)
target_sources(app PRIVATE src/event_manager.c)
target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/ble_unpair_combo.c)
target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/ext_power_generic.c)
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

Yes, I would say so. Definitely shouldn't be tied to the ZMK_BLE Kconfig setting.

required: true
label:
type: string
required: false
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

This should be required, since it's used to register the single driver instance in the driver itself:

Suggested change
required: false
required: true

const struct ext_power_generic_config *config = dev->config_info;

if (gpio_pin_set(data->gpio, config->pin, 1)) {
LOG_DBG("Failed to set ext-power control pin");
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

This should be a warning, IMHO, not a debug:

Suggested change
LOG_DBG("Failed to set ext-power control pin");
LOG_WRN("Failed to set ext-power control pin");

const struct ext_power_generic_config *config = dev->config_info;

if (gpio_pin_set(data->gpio, config->pin, 0)) {
LOG_DBG("Failed to clear ext-power control pin");
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

Ditto:

Suggested change
LOG_DBG("Failed to clear ext-power control pin");
LOG_WRN("Failed to clear ext-power control pin");

}

if (gpio_pin_configure(data->gpio, config->pin, config->flags | GPIO_OUTPUT)) {
LOG_DBG("Failed to configure ext-power control pin");
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

I would consider this an error too, and use LOG_ERR here as well

@megamind4089
Copy link
Contributor Author

megamind4089 commented Oct 9, 2020

Thanks @petejohanson for reviewing. Addressed the comments
Also rebased and squashed

app/Kconfig Outdated
@@ -95,6 +95,10 @@ config ZMK_IDLE_SLEEP_TIMEOUT

endif

config ZMK_EXT_POWER
bool "Enable support to control external power output"
default n
Copy link
Contributor

@petejohanson petejohanson Oct 9, 2020

Choose a reason for hiding this comment

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

I'm not sure defaulting n makes sense here. Without this, I think the nrfMicro won't automatically have VCC on, will it?

Copy link
Contributor Author

@megamind4089 megamind4089 Oct 10, 2020

Choose a reason for hiding this comment

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

Yep, it won't. Fixed it.

This PR adds support to control the external power output from controllers like nice!nano, nRFMicro etc

I have implemented based on my understanding of Pete suggestion on this feature.

Testing done:

    Tested by enabling and disabling the ext_power from application and verified
    Verified the application does not crash with boards that does not have ext_power support
    Note:
    I did not test this in nice!nano since I don't have the boards. Will get help from others once the behavior PR is up

Next Steps:

    Create a behavior PR to control enable/disable ext_power
Copy link
Contributor

@petejohanson petejohanson left a comment

Awesome! Looking forward to the next step on this.

@petejohanson petejohanson merged commit e1dcf15 into zmkfirmware:main Oct 10, 2020
1 check passed
@megamind4089 megamind4089 deleted the feature/ext-power branch Oct 10, 2020
MangoIV pushed a commit to MangoIV/zmk that referenced this pull request Dec 18, 2020
[RDY] Driver to control the external power output
tyalie pushed a commit to tyalie/zmk that referenced this pull request Nov 15, 2022
[RDY] Driver to control the external power output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants