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.

Sign up for GitHub

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

Jump to bottom

Wiznetif: report module power state. #2258

Merged
merged 3 commits into from
Dec 23, 2020
Merged

Wiznetif: report module power state. #2258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d1c316
Wiznetif: report module power state.
XuGuohui Dec 22, 2020
228030f
Wiznetif: minors.
XuGuohui Dec 23, 2020
62ed359
Wiznetif: always notify power state event.
XuGuohui Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions hal/network/lwip/wiznet/wiznetif.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ WizNetif::WizNetif(hal_spi_interface_t spi, pin_t cs, pin_t reset, pin_t interru
cs_(cs),
reset_(reset),
interrupt_(interrupt),
pwrState_(IF_POWER_STATE_NONE),
spiLock_(spi, WIZNET_DEFAULT_CONFIG) {

LOG(INFO, "Creating Wiznet LwIP interface");
Expand Down Expand Up @@ -265,8 +266,10 @@ err_t WizNetif::initInterface() {

hwReset();
if (!isPresent()) {
pwrState_ = IF_POWER_STATE_DOWN;
return ERR_IF;
}
pwrState_ = IF_POWER_STATE_UP;

return ERR_OK;
}
Expand Down Expand Up @@ -345,6 +348,11 @@ int WizNetif::up() {
down_ = false;
}

// Just in case. We might call up() directly after powerDown().
if (pwrState_ != IF_POWER_STATE_UP) {
notifyPowerState(IF_POWER_STATE_UP);
}

return r;
}

Expand All @@ -358,16 +366,39 @@ int WizNetif::down() {
}

int WizNetif::powerUp() {
return 0;
// FIXME: As long as the interface is initialized,
// it must have been powered up as of right now.
// The system network manager transit the interface to powering up,
// we should always notify the event to transit the interface to powered up.
notifyPowerState(IF_POWER_STATE_UP);
return SYSTEM_ERROR_NONE;
}

int WizNetif::powerDown() {
return down();
int ret = down();
// FIXME: This don't really power off the module.
// Notify the system network manager that the module is powered down
// to bypass waitInterfaceOff() as required by system sleep.
// The system network manager transit the interface to powering down,
// we should always notify the event to transit the interface to powered down.
notifyPowerState(IF_POWER_STATE_DOWN);
return ret;
}

void WizNetif::notifyPowerState(if_power_state_t state) {
pwrState_ = state;
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be:

if (pwrState_ == state) {
    return;
}
pwrState_ = state;
// Notify

So that you don't have to check externally.

Copy link
Member Author

Choose a reason for hiding this comment

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

That might miss a power state change event. Image that the pwrState_ is changed to UP on initialization, then powerUp() won't notify the power state.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's leave it as-is then 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not a good example. If the pwrState_ is changed to IF_POWER_STATE_UP somewhere but without notifying the state, somewhere else call notifyPowerState(IF_POWER_STATE_UP) won't notify the power state. Just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

While this remind me that we should always notify the power state if possible, especially in powerDown() that may block the waitInterfaceOff(). I'll revise it a little bit.

if_event evt = {};
struct if_event_power_state ev_if_power_state = {};
evt.ev_len = sizeof(if_event);
evt.ev_type = IF_EVENT_POWER_STATE;
evt.ev_power_state = &ev_if_power_state;
evt.ev_power_state->state = pwrState_;
if_notify_event(interface(), &evt, nullptr);
}

int WizNetif::getPowerState(if_power_state_t* state) const {
// TODO: implement it
return SYSTEM_ERROR_NOT_SUPPORTED;
*state = pwrState_;
return SYSTEM_ERROR_NONE;
}

int WizNetif::getNcpState(unsigned int* state) const {
Expand Down
4 changes: 4 additions & 0 deletions hal/network/lwip/wiznet/wiznetif.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,16 @@ class WizNetif : public BaseNetif {
static err_t linkOutputCb(netif* netif, pbuf* p);
err_t linkOutput(pbuf* p);

void notifyPowerState(if_power_state_t state);

private:
hal_spi_interface_t spi_;
pin_t cs_;
pin_t reset_;
pin_t interrupt_;

std::atomic<if_power_state_t> pwrState_;

os_thread_t thread_ = nullptr;
os_queue_t queue_ = nullptr;
os_semaphore_t spiSem_ = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion system/src/system_sleep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ static bool system_sleep_network_suspend(network_interface_index index) {
#endif
// Turn off the modem
network_off(index, 0, 0, NULL);
LOG(TRACE, "Waiting interface to be off...");
LOG(TRACE, "Waiting interface %d to be off...", (int)index);
// There might be up to 30s delay to turn off the modem for particular platforms.
network_wait_off(index, 120000/*ms*/, nullptr);
return resume;
Expand Down