Fix console scroll - comment on how scrolling works (#1448)

* Force console scroll area to be multiple of line height. Tons of comments.
* Resize POCSAG console to fill height.
* Make scoll behavior comment clearer

firmware/application/apps/pocsag_app.hpp

@@ -281,8 +281,9 @@ class POCSAGAppView : public View {
         {22 * 8, 1 * 16, 8 * 8, 20},
         "Config"};
 
+    // 54 == status bar (16) + top controls (2 * 16 + 6).
     Console console{
-        {0, 2 * 16 + 6, screen_width, screen_height - 56}};
+        {0, 2 * 16 + 6, screen_width, screen_height - 54}};
 
     MessageHandlerRegistration message_handler_packet{
         Message::ID::POCSAGPacket,

firmware/application/apps/ui_afsk_rx.hpp

@@ -94,15 +94,15 @@ class AFSKRxView : public View {
         false};
 
     Text text_debug{
-        {0 * 8, 12 + 2 * 16, 240, 16},
+        {0 * 8, 12 + 2 * 16, screen_width, 16},
         "DEBUG"};
 
     Button button_modem_setup{
-        {240 - 12 * 8, 1 * 16, 96, 24},
+        {screen_width - 12 * 8, 1 * 16, 96, 24},
         "Modem setup"};
 
     Console console{
-        {0, 4 * 16, 240, 240}};
+        {0, 4 * 16, 240, screen_width}};
 
     void on_data_afsk(const AFSKDataMessage& message);
 

firmware/common/lcd_ili9341.cpp

@@ -405,13 +405,12 @@ void ILI9341::drawBMP(const ui::Point p, const uint8_t* bitmap, const bool trans
     }
 }
 
-/*
+/* Draw BMP from SD card.
-        Draw BMP from SD card.
+ * Currently supported formats:
-        Currently supported formats:
+ *     16bpp ARGB, RGB565
-                16bpp ARGB, RGB565
+ *     24bpp RGB
-                24bpp RGB
+ *     32bpp ARGB
-                32bpp ARGB
+ */
-*/
 bool ILI9341::drawBMP2(const ui::Point p, const std::filesystem::path& file) {
     File bmpimage;
     size_t file_pos = 0;

firmware/common/lcd_ili9341.hpp

@@ -90,14 +90,52 @@ class ILI9341 {
         const ui::Color foreground,
         const ui::Color background);
 
+    /*** Scrolling ***
+     * Scrolling support is implemented in the ILI9341 driver. Basically a region
+     * of the screen is set up to act as a circular buffer. The VSA (vertical scroll
+     * address) is the line that defines the "start" of the circular buffer. In our
+     * case, the driver is set up for "bottom-up" scrolling. In this mode, drawing
+     * starts at the bottom of the scroll region and draws the buffer upward.
+     * However, the whole display's address space is inverted (the screen is actually
+     * upside down in the PortaPack) so this bottom-up drawing appears to be top-down.
+     *
+     * What this means is that the line pointed to by VSA will be drawn at the top
+     * of the scroll region and the line at VSA - 1 (wrapped) will be the bottom.
+     * Consider the following screen buffers and VSA pointers.
+     *
+     * Buffer:  Display:    Buffer:  Display:
+     * VSA > A      A           A       C       NB: VSA points to the "Top"
+     *       B      B           B       D           of the rendered output.
+     *       C      C     VSA > C       A
+     *       D      D           D       B
+     */
+
+    /* Sets the top and bottom lines of the scrolling region. */
     void scroll_set_area(const ui::Coord top_y, const ui::Coord bottom_y);
-    ui::Coord scroll_set_position(const ui::Coord position);
-    ui::Coord scroll(const int32_t delta);
-    ui::Coord scroll_area_y(const ui::Coord y) const;
     void scroll_disable();
 
-    constexpr ui::Dim width() const { return 240; }
+    /* Sets VSA. This an offset from top_y not a screen coordinate. */
-    constexpr ui::Dim height() const { return 320; }
+    ui::Coord scroll_set_position(const ui::Coord position);
+
+    /* Relative adjustment to VSA. Positive value will cause output
+     * to scoll "down", negative values will cause the output to scroll "up". */
+    ui::Coord scroll(const int32_t delta);
+
+    /* Gets a screen coordinate from a scroll region offset. Values are wrapped
+     * so the offset is wrapped within the bounds of the scroll region. The
+     * specified offset is applied to VSA and then offset by the scroll region
+     * top to produce a screen coordinate.
+     * Consider the following screen buffers and VSA pointer. Note the offsets.
+     *
+     * VSA > A = +0          A = +2
+     *       B = +1          B = +3   NB: As before, VSA is always the "top"
+     *       C = +2    VSA > C = +0       and line at VSA - 1 will be the
+     *       D = +3          D = +1       "bottom" or the max 'y' offset.
+     */
+    ui::Coord scroll_area_y(const ui::Coord y) const;
+
+    constexpr ui::Dim width() const { return ui::screen_width; }
+    constexpr ui::Dim height() const { return ui::screen_height; }
     constexpr ui::Rect screen_rect() const { return {0, 0, width(), height()}; }
 
     void draw_pixels(const ui::Rect r, const ui::Color* const colors, const size_t count);

firmware/common/ui_widget.cpp

@@ -494,7 +494,9 @@ void BigFrequency::paint(Painter& painter) {
     const auto rect = screen_rect();
 
     // Erase
-    painter.fill_rectangle({{0, rect.location().y()}, {240, 52}}, ui::Color::black());
+    painter.fill_rectangle(
+        {{0, rect.location().y()}, {screen_width, 52}},
+        ui::Color::black());
 
     // Prepare digits
     if (!_frequency) {
@@ -612,10 +614,10 @@ void Console::write(std::string message) {
     if (!hidden() && visible()) {
         const Style& s = style();
         const Font& font = s.font;
-        const auto rect = screen_rect();
+        auto rect = screen_rect();
         ui::Color pen_color = s.foreground;
 
-        for (const auto c : message) {
+        for (auto c : message) {
             if (escape) {
                 if (c < std::size(term_colors))
                     pen_color = term_colors[(uint8_t)c];
@@ -628,12 +630,13 @@ void Console::write(std::string message) {
                 } else if (c == '\x1B') {
                     escape = true;
                 } else {
-                    const auto glyph = font.glyph(c);
+                    auto glyph = font.glyph(c);
-                    const auto advance = glyph.advance();
+                    auto advance = glyph.advance();
-                    if ((pos.x() + advance.x()) > rect.width()) {
+                    // Would drawing next character be off the end? Newline.
+                    if ((pos.x() + advance.x()) > rect.width())
                         crlf();
-                    }
+
-                    const Point pos_glyph{
+                    Point pos_glyph{
                         rect.left() + pos.x(),
                         display.scroll_area_y(pos.y())};
                     display.draw_glyph(pos_glyph, glyph, pen_color, s.background);
@@ -649,7 +652,6 @@ void Console::write(std::string message) {
 
 void Console::writeln(std::string message) {
     write(message + "\n");
-    // crlf();
 }
 
 void Console::paint(Painter&) {
@@ -659,15 +661,23 @@ void Console::paint(Painter&) {
 void Console::on_show() {
     enable_scrolling(true);
     clear();
-    // visible = true;
 }
 
 bool Console::scrolling_enabled = false;
 
 void Console::enable_scrolling(bool enable) {
     if (enable) {
-        const auto screen_r = screen_rect();
+        auto sr = screen_rect();
-        display.scroll_set_area(screen_r.top(), screen_r.bottom());
+        auto line_height = style().font.line_height();
+
+        // Count full lines that can fit in console's rectangle.
+        auto max_lines = sr.height() / line_height;  // NB: int division to floor.
+
+        // The scroll area must be a multiple of the line_height
+        // or some lines will end up vertically truncated.
+        scroll_height = max_lines * line_height;
+
+        display.scroll_set_area(sr.top(), sr.top() + scroll_height);
         display.scroll_set_position(0);
         scrolling_enabled = true;
     } else {
@@ -678,28 +688,37 @@ void Console::enable_scrolling(bool enable) {
 
 void Console::on_hide() {
     /* TODO: Clear region to eliminate brief flash of content at un-shifted
-     * position?
+     * position? */
-     */
     enable_scrolling(false);
-    // visible = false;
 }
 
 void Console::crlf() {
     if (hidden() || !visible()) return;
 
-    const Style& s = style();
+    const auto& s = style();
-    const auto sr = screen_rect();
+    auto sr = screen_rect();
-    const auto line_height = s.font.line_height();
+    auto line_height = s.font.line_height();
-    pos = {0, pos.y() + line_height};
-    const int32_t y_excess = pos.y() + line_height - sr.height();
-    if (y_excess > 0) {
-        if (!scrolling_enabled) {
-            enable_scrolling(true);
-        }
-        display.scroll(-y_excess);
-        pos = {pos.x(), pos.y() - y_excess};
 
-        const Rect dirty{sr.left(), display.scroll_area_y(pos.y()), sr.width(), line_height};
+    // Advance to the next line (\n) position and "carriage return" x to 0.
+    pos = {0, pos.y() + line_height};
+
+    if (pos.y() >= scroll_height) {
+        // Line is past off the "bottom", need to scroll.
+        if (!scrolling_enabled)
+            enable_scrolling(true);
+
+        // See the notes in lcd_ili9341.hpp about how scrolling works.
+        // The gist is that VSA will be moved to scroll the "top" off the
+        // screen. The drawing code uses 'scroll_area_y' to get the actual
+        // screen coordinate based on VSA. The "bottom" line is *always*
+        // at 'VSA + ((max_lines - 1) * line_height)' and so is constant.
+        pos = {0, scroll_height - line_height};
+
+        // Scroll off the "top" line.
+        display.scroll(-line_height);
+
+        // Clear the new line at the "bottom".
+        Rect dirty{sr.left(), display.scroll_area_y(pos.y()), sr.width(), line_height};
         display.fill_rectangle(dirty, s.background);
     }
 }

firmware/common/ui_widget.hpp

@@ -332,8 +332,8 @@ class Console : public Widget {
     void on_hide() override;
 
    private:
-    // bool visible = false;
     Point pos{0, 0};
+    Dim scroll_height = 0;
     std::string buffer{};
     static bool scrolling_enabled;