Modify the uart_buffer code to support reading
For reads, we push one character at a time and then read multiple ones.
We want to do this for the camera board.
Change-Id: I8a32dcaf0aa822a5642f388c58e02e17564203f3
diff --git a/aos/containers/BUILD b/aos/containers/BUILD
index c139195..3e24fab 100644
--- a/aos/containers/BUILD
+++ b/aos/containers/BUILD
@@ -1,5 +1,7 @@
package(default_visibility = ["//visibility:public"])
+load("//tools:environments.bzl", "mcu_cpus")
+
cc_library(
name = "ring_buffer",
hdrs = [
@@ -41,6 +43,7 @@
hdrs = [
"sized_array.h",
],
+ compatible_with = mcu_cpus,
)
cc_test(
diff --git a/motors/peripheral/BUILD b/motors/peripheral/BUILD
index ef2e833..2c71973 100644
--- a/motors/peripheral/BUILD
+++ b/motors/peripheral/BUILD
@@ -71,6 +71,7 @@
visibility = ["//visibility:public"],
deps = [
":uart_buffer",
+ "//aos/containers:sized_array",
"//motors:util",
"//motors/core",
"//third_party/GSL",
diff --git a/motors/peripheral/uart.cc b/motors/peripheral/uart.cc
index 3f7ddaf..9cd4014 100644
--- a/motors/peripheral/uart.cc
+++ b/motors/peripheral/uart.cc
@@ -39,18 +39,24 @@
// M_UART_RIE /* Enable RX interrupt or DMA */
// Also set in C5: M_UART_TDMAS /* Do DMA for TX */ |
// M_UART_RDMAS /* Do DMA for RX */
- c2_value_ = M_UART_TE;
+ c2_value_ = 0;
module_->C2 = c2_value_;
module_->PFIFO =
M_UART_TXFE /* Enable TX FIFO */ | M_UART_RXFE /* Enable RX FIFO */;
module_->CFIFO =
M_UART_TXFLUSH /* Flush TX FIFO */ | M_UART_RXFLUSH /* Flush RX FIFO */;
+ c2_value_ = M_UART_TE | M_UART_RE;
+ module_->C2 = c2_value_;
// TODO(Brian): Adjust for DMA?
module_->TWFIFO = tx_fifo_size_ - 1;
- module_->RWFIFO = rx_fifo_size_ - 1;
+ module_->RWFIFO = 1;
}
void Uart::DoWrite(gsl::span<const char> data) {
+ // In theory, we could be more efficient about this by writing the number of
+ // bytes we know there's space for and only calling SpaceAvailable() (or
+ // otherwise reading S1) before the final one. In practice, the FIFOs are so
+ // short on this part it probably won't help anything.
for (int i = 0; i < data.size(); ++i) {
while (!SpaceAvailable()) {
}
@@ -58,38 +64,91 @@
}
}
-Uart::~Uart() { DisableTransmitInterrupt(); }
+aos::SizedArray<char, 4> Uart::DoRead() {
+ // In theory, we could be more efficient about this by reading the number of
+ // bytes we know to be accessible and only calling DataAvailable() (or
+ // otherwise reading S1) before the final one. In practice, the FIFOs are so
+ // short on this part it probably won't help anything.
+ aos::SizedArray<char, 4> result;
+ while (DataAvailable() && !result.full()) {
+ result.push_back(ReadCharacter());
+ }
+ return result;
+}
+
+Uart::~Uart() {
+ DoDisableTransmitInterrupt();
+ DoDisableReceiveInterrupt();
+}
+
+InterruptBufferedUart::~InterruptBufferedUart() {
+ uart_.DisableReceiveInterrupt(DisableInterrupts());
+}
void InterruptBufferedUart::Initialize(int baud_rate) {
uart_.Initialize(baud_rate);
+ {
+ DisableInterrupts disable_interrupts;
+ uart_.EnableReceiveInterrupt(disable_interrupts);
+ }
}
void InterruptBufferedUart::Write(gsl::span<const char> data) {
DisableInterrupts disable_interrupts;
- uart_.EnableTransmitInterrupt();
- static_assert(buffer_.size() >= 8,
- "Need enough space to not turn the interrupt off each time");
+ uart_.EnableTransmitInterrupt(disable_interrupts);
while (!data.empty()) {
- const int bytes_written = buffer_.PushSpan(data);
+ const int bytes_written = transmit_buffer_.PushSpan(data);
data = data.subspan(bytes_written);
WriteCharacters(data.empty(), disable_interrupts);
ReenableInterrupts{&disable_interrupts};
}
}
-void InterruptBufferedUart::WriteCharacters(bool disable_empty,
- const DisableInterrupts &) {
+gsl::span<char> InterruptBufferedUart::Read(gsl::span<char> buffer) {
+ size_t bytes_read = 0;
+ {
+ DisableInterrupts disable_interrupts;
+ const gsl::span<const char> read_data =
+ receive_buffer_.PopSpan(buffer.size());
+ std::copy(read_data.begin(), read_data.end(), buffer.begin());
+ bytes_read += read_data.size();
+ }
+ {
+ DisableInterrupts disable_interrupts;
+ const gsl::span<const char> read_data =
+ receive_buffer_.PopSpan(buffer.size() - bytes_read);
+ std::copy(read_data.begin(), read_data.end(),
+ buffer.subspan(bytes_read).begin());
+ bytes_read += read_data.size();
+ }
+ return buffer.subspan(0, bytes_read);
+}
+
+void InterruptBufferedUart::WriteCharacters(
+ bool disable_empty, const DisableInterrupts &disable_interrupts) {
while (true) {
- if (buffer_.empty()) {
+ if (transmit_buffer_.empty()) {
if (disable_empty) {
- uart_.DisableTransmitInterrupt();
+ uart_.DisableTransmitInterrupt(disable_interrupts);
}
return;
}
if (!uart_.SpaceAvailable()) {
return;
}
- uart_.WriteCharacter(buffer_.PopSingle());
+ uart_.WriteCharacter(transmit_buffer_.PopSingle());
+ }
+}
+
+void InterruptBufferedUart::ReadCharacters(const DisableInterrupts &) {
+ while (true) {
+ if (receive_buffer_.full()) {
+ return;
+ }
+ if (!uart_.DataAvailable()) {
+ return;
+ }
+ receive_buffer_.PushSingle(uart_.ReadCharacter());
}
}
diff --git a/motors/peripheral/uart.h b/motors/peripheral/uart.h
index ca40ab7..ffb1529 100644
--- a/motors/peripheral/uart.h
+++ b/motors/peripheral/uart.h
@@ -1,6 +1,7 @@
#ifndef MOTORS_PERIPHERAL_UART_H_
#define MOTORS_PERIPHERAL_UART_H_
+#include "aos/containers/sized_array.h"
#include "motors/core/kinetis.h"
#include "motors/peripheral/uart_buffer.h"
#include "motors/util.h"
@@ -25,22 +26,59 @@
DoWrite(data);
}
+ // Returns all the data which is currently available.
+ aos::SizedArray<char, 4> Read(const DisableInterrupts &) {
+ return DoRead();
+ }
+
bool SpaceAvailable() const { return module_->S1 & M_UART_TDRE; }
// Only call this if SpaceAvailable() has just returned true.
void WriteCharacter(char c) { module_->D = c; }
- void EnableTransmitInterrupt() {
+ bool DataAvailable() const { return module_->S1 & M_UART_RDRF; }
+ // Only call this if DataAvailable() has just returned true.
+ char ReadCharacter() { return module_->D; }
+
+ // TODO(Brian): These APIs for enabling/disabling interrupts aren't quite
+ // right. Redo them some time. Some issues:
+ // * They get called during initialization/destruction time, which means
+ // interrupts don't really need to be disabled because everything is
+ // singlethreaded.
+ // * Often, several C2 modifications are made in a single
+ // interrupts-disabled section. These could be batched to reduce
+ // peripheral writes. Sometimes, no modifications are made at all, in
+ // which case there doesn't even need to be a single write.
+
+ void EnableTransmitInterrupt(const DisableInterrupts &) {
c2_value_ |= M_UART_TIE;
module_->C2 = c2_value_;
}
- void DisableTransmitInterrupt() {
- c2_value_ &= ~M_UART_TIE;
+ void DisableTransmitInterrupt(const DisableInterrupts &) {
+ DoDisableTransmitInterrupt();
+ }
+
+ void EnableReceiveInterrupt(const DisableInterrupts &) {
+ c2_value_ |= M_UART_RIE;
module_->C2 = c2_value_;
}
+ void DisableReceiveInterrupt(const DisableInterrupts &) {
+ DoDisableReceiveInterrupt();
+ }
+
private:
+ void DoDisableTransmitInterrupt() {
+ c2_value_ &= ~M_UART_TIE;
+ module_->C2 = c2_value_;
+ }
+ void DoDisableReceiveInterrupt() {
+ c2_value_ &= ~M_UART_RIE;
+ module_->C2 = c2_value_;
+ }
+
void DoWrite(gsl::span<const char> data);
+ aos::SizedArray<char, 4> DoRead();
KINETISK_UART_t *const module_;
const int module_clock_frequency_;
@@ -51,27 +89,37 @@
};
// Interrupt-based buffered interface to a UART.
+// TODO(Brian): Move DisableInterrupts calls up to the caller of this.
class InterruptBufferedUart {
public:
InterruptBufferedUart(KINETISK_UART_t *module, int module_clock_frequency)
: uart_(module, module_clock_frequency) {}
+ ~InterruptBufferedUart();
void Initialize(int baud_rate);
+ // Queues up the given data for immediate writing. Blocks only if the queue
+ // fills up before all of data is enqueued.
void Write(gsl::span<const char> data);
+ // Reads currently available data.
+ // Returns all the data which is currently available (possibly none);
+ // buffer is where to store the result. The return value will be a subspan of
+ // this.
+ gsl::span<char> Read(gsl::span<char> buffer);
+
// Should be called as the body of the interrupt handler.
void HandleInterrupt(const DisableInterrupts &disable_interrupts) {
WriteCharacters(true, disable_interrupts);
+ ReadCharacters(disable_interrupts);
}
private:
void WriteCharacters(bool disable_empty, const DisableInterrupts &);
+ void ReadCharacters(const DisableInterrupts &);
Uart uart_;
- UartBuffer<1024> buffer_;
-
- bool interrupt_enabled_ = false;
+ UartBuffer<1024> transmit_buffer_, receive_buffer_;
};
} // namespace teensy
diff --git a/motors/peripheral/uart_buffer.h b/motors/peripheral/uart_buffer.h
index 63dd70d..879ab5c 100644
--- a/motors/peripheral/uart_buffer.h
+++ b/motors/peripheral/uart_buffer.h
@@ -15,15 +15,25 @@
// Returns the number of characters added.
__attribute__((warn_unused_result)) int PushSpan(gsl::span<const char> data);
+ // max is the maximum size the returned spans should be.
+ // The data in the result is only valid until another method is called.
+ // Note that this may not return all available data when doing so would
+ // require wrapping around, but it will always return a non-empty span if any
+ // data is available.
+ gsl::span<const char> PopSpan(int max);
+
bool empty() const { return size_ == 0; }
+ bool full() const { return size_ == kSize; }
// This may only be called when !empty().
char PopSingle();
+ // This may only be called when !full().
+ void PushSingle(char c);
static constexpr int size() { return kSize; }
private:
- // The index at which we will push the next character.
+ // The index at which we will pop the next character.
int start_ = 0;
// How many characters we currently have.
int size_ = 0;
@@ -31,7 +41,7 @@
::std::array<char, kSize> data_;
};
-template<int kSize>
+template <int kSize>
int UartBuffer<kSize>::PushSpan(gsl::span<const char> data) {
const int end_location = (start_ + size_) % kSize;
const int remaining_end = ::std::min(kSize - size_, kSize - end_location);
@@ -52,7 +62,16 @@
return on_end + on_start;
}
-template<int kSize>
+template <int kSize>
+gsl::span<const char> UartBuffer<kSize>::PopSpan(int max) {
+ const size_t result_size = std::min(max, std::min(kSize - start_, size_));
+ const auto result = gsl::span<const char>(data_).subspan(start_, result_size);
+ start_ = (start_ + result_size) % kSize;
+ size_ -= result_size;
+ return result;
+}
+
+template <int kSize>
char UartBuffer<kSize>::PopSingle() {
const char r = data_[start_];
--size_;
@@ -60,6 +79,13 @@
return r;
}
+template <int kSize>
+void UartBuffer<kSize>::PushSingle(char c) {
+ const int end_location = (start_ + size_) % kSize;
+ data_[end_location] = c;
+ ++size_;
+}
+
} // namespace teensy
} // namespace frc971
diff --git a/motors/peripheral/uart_buffer_test.cc b/motors/peripheral/uart_buffer_test.cc
index 5ab7c98..c464f8a 100644
--- a/motors/peripheral/uart_buffer_test.cc
+++ b/motors/peripheral/uart_buffer_test.cc
@@ -181,6 +181,106 @@
ASSERT_TRUE(buffer.empty());
}
+// Tests that using PopSpan with single characters works correctly.
+TEST(UartBufferTest, PopSpanSingle) {
+ UartBuffer<3> buffer;
+ ASSERT_FALSE(buffer.full());
+ buffer.PushSingle(1);
+ ASSERT_FALSE(buffer.full());
+ buffer.PushSingle(2);
+ ASSERT_FALSE(buffer.full());
+
+ {
+ const auto result = buffer.PopSpan(1);
+ ASSERT_EQ(1u, result.size());
+ EXPECT_EQ(1u, result[0]);
+ }
+
+ ASSERT_FALSE(buffer.full());
+ buffer.PushSingle(3);
+ ASSERT_FALSE(buffer.full());
+ buffer.PushSingle(4);
+ ASSERT_TRUE(buffer.full());
+
+ {
+ const auto result = buffer.PopSpan(1);
+ ASSERT_EQ(1u, result.size());
+ EXPECT_EQ(2u, result[0]);
+ }
+
+ {
+ const auto result = buffer.PopSpan(1);
+ ASSERT_EQ(1u, result.size());
+ EXPECT_EQ(3u, result[0]);
+ }
+
+ {
+ const auto result = buffer.PopSpan(1);
+ ASSERT_EQ(1u, result.size());
+ EXPECT_EQ(4u, result[0]);
+ }
+}
+
+// Tests that using PopSpan with multiple characters works correctly.
+TEST(UartBufferTest, PopSpanMultiple) {
+ UartBuffer<1024> buffer;
+ for (int i = 0; i < 10; ++i) {
+ buffer.PushSingle(i);
+ }
+ ASSERT_TRUE(buffer.PopSpan(0).empty());
+ {
+ const auto result = buffer.PopSpan(5);
+ ASSERT_EQ(5u, result.size());
+ for (int i = 0; i < 5; ++i) {
+ EXPECT_EQ(static_cast<char>(i), result[i]);
+ }
+ }
+ {
+ const auto result = buffer.PopSpan(10);
+ ASSERT_EQ(5u, result.size());
+ for (int i = 0; i < 5; ++i) {
+ EXPECT_EQ(static_cast<char>(i + 5), result[i]);
+ }
+ }
+ ASSERT_TRUE(buffer.PopSpan(5).empty());
+ ASSERT_TRUE(buffer.PopSpan(3000).empty());
+ ASSERT_TRUE(buffer.PopSpan(0).empty());
+}
+
+// Tests that using PopSpan with multiple characters works correctly when
+// wrapping.
+TEST(UartBufferTest, PopSpanWrapMultiple) {
+ UartBuffer<10> buffer;
+ for (int i = 0; i < 10; ++i) {
+ buffer.PushSingle(i);
+ }
+ ASSERT_TRUE(buffer.PopSpan(0).empty());
+ {
+ const auto result = buffer.PopSpan(5);
+ ASSERT_EQ(5u, result.size());
+ for (int i = 0; i < 5; ++i) {
+ EXPECT_EQ(static_cast<char>(i), result[i]);
+ }
+ }
+ for (int i = 0; i < 5; ++i) {
+ buffer.PushSingle(20 + i);
+ }
+ {
+ const auto result = buffer.PopSpan(10);
+ ASSERT_EQ(5u, result.size());
+ for (int i = 0; i < 5; ++i) {
+ EXPECT_EQ(static_cast<char>(i + 5), result[i]);
+ }
+ }
+ {
+ const auto result = buffer.PopSpan(10);
+ ASSERT_EQ(5u, result.size());
+ for (int i = 0; i < 5; ++i) {
+ EXPECT_EQ(static_cast<char>(i + 20), result[i]);
+ }
+ }
+}
+
} // namespace testing
} // namespace teensy
} // namespace frc971