drivers/cc110x: add power off (sleep) functions #12294

kYc0o · 2019-09-24T10:05:40Z

Contribution description

This PR attempts to re-add the sleep functions which were removed due to the driver complete rewrite. Unfortunately, I don't have the required hardware to perform an actual power consumption test. I can gladly make changes if someone has means to test the power consumption.

Testing procedure

Use the provided commands on the test application.

Issues/PRs references

Follow up #10340

drivers/cc110x/include/cc110x_communication.h

benpicco · 2019-09-24T11:08:45Z

drivers/cc110x/cc110x_communication.c

+
+    /*
+     * Set CS pin high so the device gets de-selected, and thus
+     * the sleep mode transition takes place


This already happens in cc110x_cmd() which calls spi_transfer_bytes() which de-selects the CS GPIO at the end.

Oh right, I had it on a previous version, will remove it.

maribu

More feedback and testing next week, I'm currently on vacation. I like the PR however very much, that feature is highly needed. 👍

maribu · 2019-09-24T15:56:38Z

drivers/cc110x/cc110x_communication.c

+
+    cc110x_cmd(dev, CC110X_STROBE_IDLE);
+    dev->state = CC110X_STATE_IDLE;
+}


The approach here seems to be unreliable to me. You could just issue the IDLE strobe and afterwards be 100% sure what the transceiver state is, as otherwise you rely on the driver state to always reflect the internal transceiver state. The driver tries to actually do this, but I would not try to rely on this when not needed

drivers/cc110x/include/cc110x_communication.h

maribu · 2019-09-24T16:03:59Z

tests/driver_cc110x/sc_cc110x.c

+        case 1:
+            for (unsigned i = 0; i < CC110X_NUM; i++){
+                printf("Waking up CC110x #%u:\n", i);
+                cc110x_power_on(&_cc110x_devs[i]);


This will not work, as this function was never be intended to be used that way. It will only make sure the transceiver has enough time to stabilise crystal and power, so that the following SPI transfers are working.

The function name is very unfortunate. I think the current cc110x_power_on() should be renamed, so that the name can be used for powering on the transceiver.

I think that a call to what currently is cc110x_power_on() and to cc110x_rx() would be sufficient to bring the transceiver back online, provided that the configuration registers keep their content. But I'd like to check the data sheet to confrim this

maribu · 2019-09-24T16:04:40Z

tests/driver_cc110x/sc_cc110x.c

+        case 1:
+            for (unsigned i = 0; i < CC110X_NUM; i++){
+                printf("Waking up CC110x #%u:\n", i);
+                cc110x_power_on(&_cc110x_devs[i]);


This will not work, as this function was never be intended to be used that way. It will only make sure the transceiver has enough time to stabilise crystal and power, so that the following SPI transfers are working.

The function name is very unfortunate. I think the current cc110x_power_on() should be renamed, so that the name can be used for powering on the transceiver.

I think that a call to what currently is cc110x_power_on() and to cc110x_rx() would be sufficient to bring the transceiver back online, provided that the configuration registers keep their content. But I'd like to check the data sheet to confrim this

maribu · 2019-09-24T18:53:22Z

Before I forget: The user facing API should be in drivers/include/cc110x.h. All.other headers are for driver internal stuff. cc110x_communication.h was ment for (high level SPI) communication with the transceiver.

maribu · 2019-09-29T14:35:39Z

OK, the datasheet says that all relevant config registers retain their values during sleep mode. So waking up the transceiver could be implemented as trivial as this:

int cc110x_wake_up(cc110x_t *dev) {
    if (cc110x_acquire(dev) != SPI_OK) {
        return -EIO;
    }
    cc110x_power_on(dev);
    cc110x_enter_rx_mode(dev);
    cc110x_release(dev);
    return 0;
}

But I'm not sure if this should actually be user facing API, as instead the common netdev driver interface (netdev_driver_t::set() and netdev_driver_t::get()) could be used like this:

    const netopt_state_t sleep = NETOPT_STATE_SLEEP;
    netdev_driver_t::set(NETOPT_STATE, &sleep, sizeof(sleep));

I think this way the user could use ifconfig to put the device to sleep mode and wake it back up.

kYc0o · 2019-10-08T14:00:37Z

I'd say both are necessary. At least on my application all the netdev layer puts some overhead with very little benefit.

I'm reworking the PR to reflect your comments, will push shortly.

kYc0o · 2019-10-08T16:17:09Z

I was trying to add the netdev layer for this but it seems that the whole driver is not using it for several things, e.g. if I set the sleep mode with netdev_driver_t::set(NETOPT_STATE, &sleep, sizeof(sleep)) I suppose there should be also something like:

const netopt_state_t rx = NETOPT_STATE_RX;
netdev_driver_t::set(NETOPT_STATE, &rx, sizeof(rx));

So I propose I expose the direct API driver and use it on the example, and then a rework to extend the usage for netopt for the most used modes (TX, RX, IDLE, SLEEP).

maribu · 2019-10-08T17:57:07Z

I don't think that setting other states than sleep makes sense, e.g. what data should the device send when the state is set to TX?

benpicco · 2019-10-08T18:22:21Z

what data should the device send when the state is set to TX?

Actually if you set NETOPT_PRELOADING (and the driver implements it), a packet would not be sent until you set the state to TX.

I don't know what this is used for and setting all the other states seems odd to me too. I just copied what the other drivers were doing, but thinking about it it's probably better not to set the state if it makes no sense.

miri64 · 2019-10-08T18:31:08Z

I don't know what this is used for and setting all the other states seems odd to me too.

It was originally introduced to allow for IEEE 802.15.4e (TSCH).

miri64 · 2019-10-08T18:31:43Z

(I don't know if it was ever used for that)

kYc0o · 2019-10-09T09:01:36Z

I was referring to something like this which is not implemented in the same way for the cc110x. The states are set and tracked using that function.

maribu · 2019-10-09T10:25:25Z

I was referring to something like this

Not that this function is basically empty. It never sets the state with two exceptions: For the reset and for sending a preloaded frame.

Before implementing this: Please keep in mind that this will break the state machine of the driver, so that significant effort (and overhead in ROM and RAM) would be needed to fix it. And so far no reason was presented on why this would be needed. So what is the whole point in this?

maribu · 2019-10-09T10:39:33Z

So summing up: I think that only setting NETOPT_STATE_SLEEP and NETOPT_STATE_IDLE should be settable, and setting everything else as state should simple return -ENOTSUP.

The following semantics make sense to me

set(NETOPT_STATE, NETOPT_STATE_SLEEP):
- If already sleeping: Return success and ignore
- Else: Abort any pending TX or RX and go to sleep
set(NETOPT_STATE, NETOPT_STATE_IDLE)
- For any state except sleeping: Return success and ignore
- If currently sleeping: Wake up the transceiver

maribu · 2019-10-16T08:50:42Z

OK, the discussion about how to make this feature available under a common API seems to stall this PR. It also seems that more discussion on this aspect is needed.

So I think we have two options to handle this:

Get the sleep/awake functionality in with a custom API for now, add the common API later on when the path for that is clear
Wait for the discussion on the common API to conclude before continuing to work on this API

I'm personally fine with both options.

benpicco · 2020-03-13T14:53:03Z

I see no problem in merging this as-is and adding the netdev integration later.

miri64 · 2020-06-25T12:48:33Z

I see no problem in merging this as-is and adding the netdev integration later.

So what is the hold up?

kYc0o · 2020-06-25T14:41:56Z

Well, on my side, at least a rebase and small refactoring of some things I found. I'll try to do it ASAP, but don't wait for it for this release IMHO.

maribu

Looks good to me. Two inline comments. Please squash right away.

maribu · 2020-06-25T14:36:27Z

drivers/cc110x/cc110x.c

@@ -175,3 +175,34 @@ int cc110x_set_channel(cc110x_t *dev, uint8_t channel)
    dev->netdev.event_callback(&dev->netdev, NETDEV_EVENT_FHSS_CHANGE_CHANNEL);
    return 0;
 }
+
+int cc110x_wakeup(cc110x_t *dev)


I'd rather have this called cc110x_power_on() to match the name of cc110x_power_off(). Maybe the existing cc110x_power_on() function could be renamed to cc110x_low_level_power_on(), or cc110x_init_chip(), or ...?

maribu · 2020-06-25T14:44:51Z

drivers/cc110x/cc110x.c

+    if (cc110x_acquire(dev) != SPI_OK) {
+        return -EIO;
+    }
+
+    cc110x_power_on(dev);


I'm not sure if this is safe to do. The current cc110x_power_on() will reconfigure the CS pin as a GPIO to signal the chip to boot up, wait for the upper bound the chip needs to boot, and than perform spi_init_cs() again.

Calling spi_init_cs() while the SPI interface is acquired might cause issues. Even if all SPI drivers would support this, cc110x_power_on() takes ages. So this would block the SPI bus, which might be shared with other devices, for way longer than usual operation. This could, thus, prevent the use of the power saving features in real-time scenarios.

Yeah you're right, I'll check what's the current status at this point, to see if a "reinit" is really needed.

stale · 2021-01-30T10:56:57Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

kYc0o added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Sep 24, 2019

kYc0o requested a review from maribu September 24, 2019 10:05

miri64 changed the title ~~Pr/add cc110x poweroff~~ drivers/cc110x: add power off (sleep) functions Sep 24, 2019

benpicco reviewed Sep 24, 2019

View reviewed changes

drivers/cc110x/include/cc110x_communication.h Outdated Show resolved Hide resolved

benpicco reviewed Sep 24, 2019

View reviewed changes

maribu reviewed Sep 24, 2019

View reviewed changes

kYc0o added 2 commits October 8, 2019 15:58

drivers/cc110x: add power off (sleep) functions

0bce97b

tests/driver_cc110x: add sleep and wake-up commands

96d887b

kYc0o added 2 commits October 8, 2019 18:14

fixup! drivers/cc110x: add power off (sleep) functions

9ecce7f

fixup! tests/driver_cc110x: add sleep and wake-up commands

6f07dab

kuguhome-infra force-pushed the pr/add_cc110x_poweroff branch from 1534136 to 6f07dab Compare October 8, 2019 16:18

maribu mentioned this pull request Oct 16, 2019

Extensebility of netdev_driver_t API #12469

Closed

maribu reviewed Jun 25, 2020

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 30, 2021

stale bot closed this Mar 19, 2021

maribu mentioned this pull request Mar 25, 2021

drivers/cc110x: add power off (sleep) functions #16232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/cc110x: add power off (sleep) functions #12294

drivers/cc110x: add power off (sleep) functions #12294

kYc0o commented Sep 24, 2019

benpicco Sep 24, 2019

kYc0o Sep 24, 2019

maribu left a comment

maribu Sep 24, 2019

maribu Sep 24, 2019

maribu Sep 24, 2019

maribu commented Sep 24, 2019

maribu commented Sep 29, 2019

kYc0o commented Oct 8, 2019

kYc0o commented Oct 8, 2019

maribu commented Oct 8, 2019

benpicco commented Oct 8, 2019

miri64 commented Oct 8, 2019

miri64 commented Oct 8, 2019

kYc0o commented Oct 9, 2019

maribu commented Oct 9, 2019

maribu commented Oct 9, 2019

maribu commented Oct 16, 2019

benpicco commented Mar 13, 2020

miri64 commented Jun 25, 2020

kYc0o commented Jun 25, 2020

maribu left a comment

maribu Jun 25, 2020

maribu Jun 25, 2020

kYc0o Jul 28, 2020

stale bot commented Jan 30, 2021

drivers/cc110x: add power off (sleep) functions #12294

drivers/cc110x: add power off (sleep) functions #12294

Conversation

kYc0o commented Sep 24, 2019

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu commented Sep 24, 2019

maribu commented Sep 29, 2019

kYc0o commented Oct 8, 2019

kYc0o commented Oct 8, 2019

maribu commented Oct 8, 2019

benpicco commented Oct 8, 2019

miri64 commented Oct 8, 2019

miri64 commented Oct 8, 2019

kYc0o commented Oct 9, 2019

maribu commented Oct 9, 2019

maribu commented Oct 9, 2019

maribu commented Oct 16, 2019

benpicco commented Mar 13, 2020

miri64 commented Jun 25, 2020

kYc0o commented Jun 25, 2020

maribu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jan 30, 2021