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 esp32 select race conditions. #774

Closed
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
3 changes: 1 addition & 2 deletions applications/hub_test/targets/linux.x86/main.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,7 @@ class Receiver : public CanHubPortInterface
numFrames_ = numUnknownFrames_ = numInOrder_ = numOutOfOrder_ =
numMissed_ = 0;

ret += StringPrintf("|RTT %.1f msec +- %.1f\n",
rttUsec_.favg()/1000, rttUsec_.stddev() / 1000);
ret += StringPrintf("|RTT %s\n", rttUsec_.debug_string().c_str());

rttUsec_.clear();
return ret;
Expand Down
8 changes: 7 additions & 1 deletion src/executor/Executor.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,13 @@ void ExecutorBase::wait_with_select(long long wait_length)
fd_set fd_r(selectRead_);
fd_set fd_w(selectWrite_);
fd_set fd_x(selectExcept_);
if (!empty()) {
// We will check the queue for any prior wakeups after this call. If we
// already processed the executables, the wakeup is not necessary. Without
// this clear, there would always be two select() iterations happening when
// we are done with work and can go to sleep.
selectHelper_.clear_wakeup();
if (!empty())
{
wait_length = 0;
}
long long max_sleep = MSEC_TO_NSEC(config_executor_max_sleep_msec());
Expand Down
115 changes: 115 additions & 0 deletions src/openlcb/LatencyTestConsumer.hxx
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/** \copyright
* Copyright (c) 2024, Balazs Racz
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* - Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
*
* - Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*
* \file LatencyTestConsumer.hxx
*
* Utility class for determining the response latency of a node.
*
* @author Balazs Racz
* @date 2 Feb 2024
*/

#ifndef _OPENLCB_LATENCYTESTCONSUMER_HXX_
#define _OPENLCB_LATENCYTESTCONSUMER_HXX_

#include "openlcb/EventHandlerTemplates.hxx"

namespace openlcb
{

class LatencyTestConsumer : public SimpleEventHandler
{
public:
/// To complete the hook, call the notifiable.
using HookFn = std::function<void(Notifiable *)>;

LatencyTestConsumer(Node *node, HookFn hook = nullptr)
: node_(node)
, hook_(hook)
{
EventRegistry::instance()->register_handler(
EventRegistryEntry(this, EVENT_BASE), 32);
}

void handle_identify_global(const EventRegistryEntry &registry_entry,
EventReport *event, BarrierNotifiable *done) override
{
AutoNotify an(done);
if (event->dst_node && event->dst_node != node_)
{
return;
}
event->event_write_helper<1>()->WriteAsync(node_,
Defs::MTI_CONSUMER_IDENTIFIED_RANGE, WriteHelper::global(),
eventid_to_buffer(EncodeRange(EVENT_BASE, 0xffffffff)),
done->new_child());
}

void handle_identify_consumer(const EventRegistryEntry &entry,
EventReport *event, BarrierNotifiable *done) override
{
event_ = event;
done_ = done;
if (hook_)
{
hook_(new TempNotifiable([this]() { reply(); }));
}
else
{
reply();
}
}

private:
void reply()
{
AutoNotify an(done_);
event_->event_write_helper<1>()->WriteAsync(node_,
Defs::MTI_CONSUMER_IDENTIFIED_UNKNOWN, WriteHelper::global(),
eventid_to_buffer(event_->event), done_->new_child());
}

/// This is within NMRA ID 1, which is not assigned. Lower four bytes are
/// the id.
static constexpr EventId EVENT_BASE = 0x0900013900000000;

/// Which node should be sending responses.
Node *node_;

/// The owner's hook will be invoked and run (asynchronous) before
/// replying..
HookFn hook_;

/// Incoming event report we are working on.
EventReport *event_;

/// Will notify this after sending the reply.
BarrierNotifiable *done_ {nullptr};
};

} // namespace openlcb

#endif // _OPENLCB_LATENCYTESTCONSUMER_HXX_
15 changes: 14 additions & 1 deletion src/os/OSSelectWakeup.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ int OSSelectWakeup::select(int nfds, fd_set *readfds,
else
{
#if OPENMRN_FEATURE_DEVICE_SELECT
// It is important that we do this select_clear() in the same
// critical section as checking pendingWakeup above. This ensures
// tht all wakeups before this clear cause sleep to be zero, and
// all wakeups after this clear will cause the event group bit to
// be set.
Device::select_clear();
#endif
}
Expand Down Expand Up @@ -189,6 +194,15 @@ void OSSelectWakeup::esp_start_select(fd_set *readfds, fd_set *writefds,
exceptFds_ = exceptfds;
exceptFdsOrig_ = *exceptfds;
FD_ZERO(exceptFds_);
if (pendingWakeup_)
{
// There is a race condition between the Executor deciding to run
// select, and the internal implementation of select() calling this
// function. Since we only get the semaphone in this call, the wakeup
// functions are noops if they hit during this window. If there was a
// missed wakeup, we repeat it.
esp_wakeup();
}
}

/// This function marks the stored semaphore as invalid which indicates no
Expand Down Expand Up @@ -226,7 +240,6 @@ void OSSelectWakeup::esp_wakeup()
/// call from within an ISR context.
void OSSelectWakeup::esp_wakeup_from_isr()
{
AtomicHolder h(this);
BaseType_t woken = pdFALSE;

// If our VFS FD is not set in the except fd_set we can exit early.
Expand Down
4 changes: 4 additions & 0 deletions src/os/OSSelectWakeup.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@ public:
#if OPENMRN_FEATURE_RTOS_FROM_ISR
void wakeup_from_isr()
{
#if defined(ESP_PLATFORM)
// On multi-core ESP32s we need to lock objects even in ISRs.
AtomicHolder h(this);
#endif
pendingWakeup_ = true;
if (inSelect_)
{
Expand Down
43 changes: 43 additions & 0 deletions src/utils/Stats.cxx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/** \copyright
* Copyright (c) 2023, Balazs Racz
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* - Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
*
* - Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*
* \file Stats.hxx
*
* Utility class for collecting statistics.
*
* @author Balazs Racz
* @date 28 Dec 2023
*/

#include "utils/Stats.hxx"

#include "utils/StringPrintf.hxx"

std::string Stats::debug_string()
{
return StringPrintf("%.1f msec +- %.1f, max %.1f\n", favg() / 1000,
stddev() / 1000, ((FloatType)max_) / 1000);
}
15 changes: 14 additions & 1 deletion src/utils/Stats.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@

#include <math.h>
#include <stdint.h>
#include <string>
#include <limits>

class Stats
{
public:
using FloatType = double;
using ValueType = int32_t;

Stats()
{
Expand All @@ -54,16 +57,21 @@ public:
sum_ = 0;
count_ = 0;
qsum_ = 0;
max_ = std::numeric_limits<ValueType>::min();
}

/// Appends a data point to the statistics.
/// @param value the data point.
void add(int32_t value)
void add(ValueType value)
{
++count_;
sum_ += value;
FloatType fval = value;
qsum_ += fval * fval;
if (value > max_)
{
max_ = value;
}
}

/// @return the average
Expand All @@ -86,9 +94,14 @@ public:
return sqrt(qsum_ * count_ - sum * sum) / count_;
}

/// Creates a half-a-line prinout of this stats object for debug purposes.
std::string debug_string();

private:
/// Number of samples added.
uint32_t count_;
/// Maximum value found since the last clear.
int32_t max_;
/// Sum of sample values added.
int64_t sum_;
/// Sum of squares of sample values added.
Expand Down
1 change: 1 addition & 0 deletions src/utils/sources
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ CXXSRCS += \
Queue.cxx \
ReflashBootloader.cxx \
ServiceLocator.cxx \
Stats.cxx \
SocketCan.cxx \
SocketClient.cxx \
constants.cxx \
Expand Down