Merge pull request #21 from DCC-EX:displayinterface-refactor

Refactored DisplayInterface, all tests passing

DisplayInterface.h

@@ -19,6 +19,7 @@
 #define DISPLAYINTERFACE_H
 
 #include "Logger.h"
+#include "Screen.h"
 
 /// @brief Class to abstract away all physical display implementation to enable multiple display types
 class DisplayInterface {
@@ -29,16 +30,9 @@ class DisplayInterface {
   /// @brief Clear the entire screen
   virtual void clearScreen() = 0;
 
-  /// @brief Display a row of text on the display
-  /// @param row Row number as specified in the SCREEN() command (not pixels)
-  /// @param text Text to be displayed on this row
-  /// @param underlined (Optional) Flag to underline this row - default false
-  /// @param column Column number to start displaying at (based on text width, not pixels)
-  virtual void displayRow(uint8_t row, const char *text, bool underlined = false, uint8_t column = 0) = 0;
-
-  /// @brief Clear the specified row
-  /// @param row Row number as specified in the SCREEN() command (not pixels)
-  virtual void clearRow(uint8_t row) = 0;
+  /// @brief Display the specified Screen on this display
+  /// @param screen Pointer to the Screen to display
+  virtual void displayScreen(Screen *screen) = 0;
 
   /// @brief Display the startup screen with software version
   /// @param version EX-Display version

DisplayManager.cpp

@@ -95,21 +95,9 @@ void DisplayManager::update(ScreenManager *screenManager) {
     }
     // Get the current screen for this display
     Screen *screen = screenManager->getScreenById(screenId);
-    // If there is one, display the rows
+    // If there is one, display it
     if (screen) {
-      // If this display needs redrawing, clear first then process rows
-      // Must set a local redraw flag here so we can clear the instance for next time
-      bool redraw = display->needsRedraw();
-      if (redraw) {
-        display->clearScreen();
-      }
-      for (ScreenRow *row = screen->getFirstScreenRow(); row; row = row->getNext()) {
-        if (row->needsRedraw() || redraw) {
-          display->displayRow(row->getId(), row->getText(), false, 0);
-        }
-      }
-      // Now we've redrawn, clear the flag
-      display->setNeedsRedraw(false);
+      display->displayScreen(screen);
     }
   }
 }

TFT_eSPIDisplay.cpp

@@ -76,6 +76,21 @@ void TFT_eSPIDisplay::clearScreen() {
   _tft->fillScreen(_backgroundColour);
 }
 
+void TFT_eSPIDisplay::displayScreen(Screen *screen) {
+  // If this display needs redrawing, clear first then process rows
+  // Must set a local redraw flag here so we can clear the instance for next time
+  if (_needsRedraw) {
+    clearScreen();
+  }
+  for (ScreenRow *row = screen->getFirstScreenRow(); row; row = row->getNext()) {
+    if (row->needsRedraw() || _needsRedraw) {
+      displayRow(row->getId(), row->getText(), false, 0);
+    }
+  }
+  // Now we've redrawn, clear the flag
+  _needsRedraw = false;
+}
+
 void TFT_eSPIDisplay::displayRow(uint8_t row, const char *text, bool underlined, uint8_t column) {
   if (text == nullptr) {
     return;

TFT_eSPIDisplay.h

@@ -65,16 +65,20 @@ class TFT_eSPIDisplay : public DisplayInterface {
   /// @brief Clear the entire screen
   void clearScreen() override;
 
+  /// @brief Display the specified Screen on this display
+  /// @param screen Pointer to the Screen to display
+  void displayScreen(Screen *screen) override;
+
   /// @brief Display a row of text on the display
   /// @param row Row number as specified in the SCREEN() command (not pixels)
   /// @param text Text to be displayed on this row
   /// @param underlined (Optional) Flag to underline this row - default false
   /// @param column (Optional) Column to start displaying the text, column being width of a character (not pixels)
-  void displayRow(uint8_t row, const char *text, bool underlined = false, uint8_t column = 0) override;
+  void displayRow(uint8_t row, const char *text, bool underlined = false, uint8_t column = 0);
 
   /// @brief Clear the specified row
   /// @param row Row number as specified in the SCREEN() command (not pixels)
-  void clearRow(uint8_t row) override;
+  void clearRow(uint8_t row);
 
   /// @brief Display the startup screen with software version
   /// @param version EX-Display version

Version.h

@@ -2,8 +2,11 @@
 #define VERSION_H
 
 // Numeric version here: major.minor.patch
-#define VERSION "0.0.20"
+#define VERSION "0.0.21"
 
+// 0.0.21 includes:
+//  - Refactor DisplayInterface to accept an entire Screen to display to let the derived classes control the entire
+//    physical display process
 // 0.0.20 includes:
 //  - Update log levels with LOG_ preface to avoid STM32 framework conflicts for ERROR
 // 0.0.19 includes:

test/integration/test_Controller/test_ControllerDisplayUpdate.cpp

@@ -62,14 +62,10 @@ TEST_F(ControllerDisplayUpdateTests, OneScreenOneDisplay) {
   // Ensure the buffer contains it as expected
   EXPECT_THAT(commandStation.buffer, testing::HasSubstr(screen0row0));
 
-  // We should always get a clearScreen() first when a display needs redrawing
-  EXPECT_CALL(*display0, clearScreen()).Times(1);
-  // Set up expectation that our display will have displayRow(uint8_t row, const char *text, bool underlined, uint8_t
-  // column) called once for each row
-  EXPECT_CALL(*display0, displayRow(Truly([=](uint8_t row) { return row == 0; }), StrEq("Screen 0 row 0"),
-                                    Truly([=](bool underlined) { return underlined == false; }),
-                                    Truly([=](uint8_t column) { return column == 0; })))
-      .Times(1);
+  // Expect displayScreen to be called for every character going through the buffer
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 0;
+  }))).Times(47);
 
   for (size_t i = 0; i < strlen(screen0row0); i++) {
     controller->update();
@@ -86,13 +82,6 @@ TEST_F(ControllerDisplayUpdateTests, OneScreenOneDisplay) {
   // Ensure the buffer contains it as expected
   EXPECT_THAT(commandStation.buffer, testing::HasSubstr(screen0row2));
 
-  // Set up expectation that our display will have displayRow(uint8_t row, const char *text, bool underlined, uint8_t
-  // column) called once for each row
-  EXPECT_CALL(*display0, displayRow(Truly([=](uint8_t row) { return row == 2; }), StrEq("Screen 0 row 2"),
-                                    Truly([=](bool underlined) { return underlined == false; }),
-                                    Truly([=](uint8_t column) { return column == 0; })))
-      .Times(1);
-
   for (size_t i = 0; i < strlen(screen0row2); i++) {
     controller->update();
   }
@@ -103,13 +92,6 @@ TEST_F(ControllerDisplayUpdateTests, OneScreenOneDisplay) {
   // Ensure the buffer contains it as expected
   EXPECT_THAT(commandStation.buffer, testing::HasSubstr(screen0row5));
 
-  // Set up expectation that our display will have displayRow(uint8_t row, const char *text, bool underlined, uint8_t
-  // column) called once for each row
-  EXPECT_CALL(*display0, displayRow(Truly([=](uint8_t row) { return row == 5; }), StrEq("Screen 0 row 5"),
-                                    Truly([=](bool underlined) { return underlined == false; }),
-                                    Truly([=](uint8_t column) { return column == 0; })))
-      .Times(1);
-
   for (size_t i = 0; i < strlen(screen0row5); i++) {
     controller->update();
   }

test/integration/test_Controller/test_ControllerInputAction.cpp

@@ -86,39 +86,68 @@ TEST_F(ControllerInputActionTests, SwitchActiveScreen) {
       .WillOnce(Invoke([this]() { controller->onInputAction(InputAction::PRESS_RIGHT); })) // Second press right
       .WillOnce(Return()); // Last will have no return with no input
 
-  // Everytime a display switches screens, a redraw should trigger clearScreen()
-  // Therefore we expect this to be called 6 times - 1 to start, then one with each switch
-  EXPECT_CALL(*display0, clearScreen()).Times(6);
-
+  // First update should mean display screen 0
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 0;
+  }))).Times(1);
+
   // Expect a single controller->update() should now ensure display is set to screen 0 (first)
   controller->update();
   EXPECT_EQ(display0->getScreenId(), 0);
 
+  // Next update after left press should mean display screen 8
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 8;
+  }))).Times(1);
+
   // Now controller->update() should action the left press
   controller->update();
 
   // Display0's screen ID should now be 8
   EXPECT_EQ(display0->getScreenId(), 8);
 
+  // Next left press should mean display screen 2
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 2;
+  }))).Times(1);
+
   // Now call controller->update() should action the left press
   controller->update();
 
   // Display0's screen ID should now be 2
   EXPECT_EQ(display0->getScreenId(), 2);
 
+  // Next left press should mean display screen 0 again
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 0;
+  }))).Times(1);
+
   // Now call controller->update() should action the left press
   controller->update();
 
   // Display0's screen ID should now be 0
   EXPECT_EQ(display0->getScreenId(), 0);
 
+  // Now the right presses should show 2 then 8
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 2;
+  }))).Times(1);
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 8;
+  }))).Times(1);
+
   // Call controller->update() twice
   controller->update();
   controller->update();
 
   // Display0's screen ID should now be 8
   EXPECT_EQ(display0->getScreenId(), 8);
 
+  // Final update should be showing 8 again
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 8;
+  }))).Times(1);
+
   controller->update();
 
   // Display0's screen ID should be unchanged at 8

test/integration/test_Display/test_DisplayScreens.cpp

@@ -53,8 +53,10 @@ TEST_F(DisplayScreenTests, CreateDisplay) {
   screenManager->updateScreen(0);
   EXPECT_EQ(screenManager->getFirstScreen()->getId(), 0);
 
-  // First update of a display with a screen should cause a clearScreen() call
-  EXPECT_CALL(*display0, clearScreen()).Times(1);
+  // An update should call displayScreen() for the display
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 0;
+  }))).Times(1);
 
   // Call controller->update() and display should still have the first screen ID
   displayManager->update(screenManager);
@@ -82,19 +84,9 @@ TEST_F(DisplayScreenTests, UpdateDisplays) {
   screenManager->getScreenById(0)->updateScreenRow(0, "Screen 0 row 0");
   screenManager->getScreenById(1)->updateScreenRow(0, "Screen 1 row 0");
 
-  // When calling displayManager->update(screenManager), both displays should have update called
-  EXPECT_CALL(*display0, displayRow(Truly([=](int row) { return row == 0; }), StrEq("Screen 0 row 0"),
-                                    Truly([=](bool underline) { return underline == false; }),
-                                    Truly([=](int column) { return column == 0; })))
-      .Times(1);
-  EXPECT_CALL(*display1, displayRow(Truly([=](int row) { return row == 0; }), StrEq("Screen 1 row 0"),
-                                    Truly([=](bool underline) { return underline == false; }),
-                                    Truly([=](int column) { return column == 0; })))
-      .Times(1);
-
-  // First update of a display with a screen should cause a clearScreen() call
-  EXPECT_CALL(*display0, clearScreen()).Times(1);
-  EXPECT_CALL(*display1, clearScreen()).Times(1);
+  // When calling displayManager->update(screenManager), both displays should have displayScreen called
+  EXPECT_CALL(*display0, displayScreen(Truly([=](Screen *screen) { return screen->getId() == 0; }))).Times(1);
+  EXPECT_CALL(*display1, displayScreen(Truly([=](Screen *screen) { return screen->getId() == 1; }))).Times(1);
 
   // Call update
   displayManager->update(screenManager);

test/mocks/MockDisplay.h

@@ -19,6 +19,7 @@
 #define MOCKDISPLAY_H
 
 #include "DisplayInterface.h"
+#include "Screen.h"
 #include <gmock/gmock.h>
 
 /// @brief Mock physical display class
@@ -28,9 +29,7 @@ class MockDisplay : public DisplayInterface {
 
   MOCK_METHOD(void, clearScreen, (), (override));
 
-  MOCK_METHOD(void, displayRow, (uint8_t row, const char *text, bool underlined, uint8_t column), (override));
-
-  MOCK_METHOD(void, clearRow, (uint8_t row), (override));
+  MOCK_METHOD(void, displayScreen, (Screen *screen), (override));
 
   MOCK_METHOD(void, displayStartupInfo, (const char *version), (override));
 

test/mocks/MockSPIDisplay.h

@@ -34,9 +34,7 @@ class MockSPIDisplay : public DisplayInterface {
 
   MOCK_METHOD(void, clearScreen, (), (override));
 
-  MOCK_METHOD(void, displayRow, (uint8_t row, const char *text, bool underlined, uint8_t column), (override));
-
-  MOCK_METHOD(void, clearRow, (uint8_t row), (override));
+  MOCK_METHOD(void, displayScreen, (Screen *screen), (override));
 
   MOCK_METHOD(void, displayStartupInfo, (const char *version), (override));
 

test/unit/test_Display/test_DisplayInterface.cpp

@@ -15,6 +15,7 @@
  *  along with this code.  If not, see <https://www.gnu.org/licenses/>.
  */
 
+#include "Screen.h"
 #include "Version.h"
 #include "test/mocks/MockDisplay.h"
 #include <gtest/gtest.h>
@@ -55,26 +56,15 @@ TEST_F(DisplayInterfaceTests, TestBasicMethods) {
 
 /// @brief Test DisplayInterface methods that should interact with a physical display
 TEST_F(DisplayInterfaceTests, TestParameterMethods) {
-  // Test methods requiring SCREEN() parameters
-  int expectRow = 2;
-  const char *expectRowText = "This is text for row 2";
-  bool expectUnderline = true;
-  int expectColumn = 1;
+  // Create a screen with nothing in it
+  Screen *screen = new Screen(0);
 
-  // Setup the expected displayRow call to validate parameters are received and called only once
-  EXPECT_CALL(*display, displayRow(Truly([=](int row) { return row == expectRow; }), StrEq(expectRowText),
-                                   Truly([=](bool underline) { return underline == expectUnderline; }),
-                                   Truly([=](int column) { return column == expectColumn; })))
-      .Times(1);
+  // Setup displayScreen expectation
+  EXPECT_CALL(*display, displayScreen(Truly([=](Screen *displayScreen) {
+    return displayScreen->getId() == 0;
+  }))).Times(1);
 
-  // Call the method that should trigger displayRow
-  display->displayRow(2, "This is text for row 2", true, 1);
-
-  // Setup the expected clearRow call
-  EXPECT_CALL(*display, clearRow(Truly([=](int row) { return row == expectRow; }))).Times(1);
-
-  // Call it
-  display->clearRow(2);
+  display->displayScreen(screen);
 
   // Make sure the screen ID is updated
   display->setScreenId(4);
@@ -84,6 +74,9 @@ TEST_F(DisplayInterfaceTests, TestParameterMethods) {
 
   // Verify all expectations were made
   testing::Mock::VerifyAndClearExpectations(display);
+
+  // Clean up
+  delete screen;
 }
 
 /// @brief Test to ensure the startup info sets the EX-Display version correctly