Update playlist file parsing, reduce allocs (#1157)

* Update playlist file parsing, reduce allocs
* Cleanup, move impl to cpp

firmware/application/CMakeLists.txt

@@ -175,6 +175,7 @@ set(CPPSRC
 	#emu_cc1101.cpp
 	rfm69.cpp
 	event_m0.cpp
+	file_reader.cpp
 	file.cpp
 	freqman.cpp
 	io_file.cpp

firmware/application/apps/ui_looking_glass_app.cpp

@@ -22,6 +22,7 @@
  */
 
 #include "ui_looking_glass_app.hpp"
+#include "convert.hpp"
 #include "file_reader.hpp"
 #include "string_format.hpp"
 
@@ -534,11 +535,15 @@ void GlassView::load_Presets() {
             if (cols.size() != 3)
                 continue;
 
-            // TODO: add some conversion helpers that take string_view.
-            presets_db.emplace_back(preset_entry{
-                std::stoi(std::string{cols[0]}),
-                std::stoi(std::string{cols[1]}),
-                trimr(std::string{cols[2]})});
+            preset_entry entry{};
+            parse_int(cols[0], entry.min);
+            parse_int(cols[1], entry.max);
+            entry.label = trimr(cols[2]);
+
+            if (entry.min == 0 || entry.max == 0 || entry.min >= entry.max)
+                continue;  // Invalid line.
+
+            presets_db.emplace_back(std::move(entry));
         }
     }
 

firmware/application/apps/ui_playlist.cpp

@@ -22,6 +22,8 @@
  */
 
 #include "ui_playlist.hpp"
+#include "convert.hpp"
+#include "file_reader.hpp"
 #include "string_format.hpp"
 
 #include "ui_fileman.hpp"
@@ -43,79 +45,54 @@ void PlaylistView::set_ready() {
 
 void PlaylistView::load_file(std::filesystem::path playlist_path) {
     File playlist_file;
-
     auto error = playlist_file.open(playlist_path.string());
-    if (!error.is_valid()) {
-        std::string line;
-        char one_char[1];
-        for (size_t pointer = 0; pointer < playlist_file.size(); pointer++) {
-            playlist_file.seek(pointer);
-            playlist_file.read(one_char, 1);
-            if ((int)one_char[0] >= ' ') {
-                line += one_char[0];
-            } else if (one_char[0] == '\n') {
-                txtline_process(line);
-                line.clear();
-            }
-        }
-        if (line.length() > 0) {
-            txtline_process(line);
+
+    if (!error) {
+        auto reader = FileLineReader(playlist_file);
+        for (const auto& line : reader) {
+            if (line.length() == 0 || line[0] == '#')
+                continue;  // Empty or comment line.
+
+            auto cols = split_string(line, ',');
+            if (cols.size() < 3)
+                continue;  // Line doesn't have enough columns.
+
+            playlist_entry entry{};
+
+            parse_int(cols[0], entry.replay_frequency);
+            parse_int(cols[2], entry.sample_rate);
+            if (entry.replay_frequency == 0 || entry.sample_rate == 0)
+                continue;  // Invalid freq or rate.
+
+            entry.replay_file = std::string{"/"} + std::string{cols[1]};
+
+            if (cols.size() == 4)  // Optional delay value.
+                parse_int(cols[3], entry.next_delay);
+
+            playlist_db.emplace_back(std::move(entry));
         }
     }
+
+    total_tracks = playlist_db.size();
     playlist_masterdb = playlist_db;
     text_track.set(to_string_dec_uint(track_number) + "/" + to_string_dec_uint(total_tracks) + " " + now_play_list_file.string());
     tracks_progressbar.set_max(total_tracks);
     button_play.focus();
-    return;
-}
-
-void PlaylistView::txtline_process(std::string& line) {
-    playlist_entry new_item;
-    size_t previous = 0;
-    auto current = std::string::npos;
-    // read freq
-    current = line.find(',');
-    if (current == std::string::npos) return;
-    errno = 0;
-    new_item.replay_frequency = strtoll(line.substr(0, current).c_str(), nullptr, 0);
-    if (new_item.replay_frequency == 0 || errno == EINVAL || errno == ERANGE)
-        return;
-    // read file
-    previous = current + 1;
-    if ((current = line.find(',', previous)) == std::string::npos) return;
-    new_item.replay_file = "/" + line.substr(previous, current - previous);
-    // read samplerate
-    previous = current + 1;
-    errno = 0;
-    new_item.sample_rate = strtoll(line.substr(previous).c_str(), nullptr, 10);
-    if (new_item.sample_rate == 0 || errno == EINVAL || errno == ERANGE)
-        return;
-    // optionnal read delay
-    current = line.find(',', previous);
-    if (current == std::string::npos) {
-        new_item.next_delay = 0;
-    } else {
-        errno = 0;
-        previous = current + 1;
-        new_item.next_delay = strtoll(line.substr(previous).c_str(), nullptr, 10);
-        if (errno == EINVAL || errno == ERANGE)
-            return;
-    }
-    playlist_db.push_back(std::move(new_item));
-    total_tracks++;
 }
 
 void PlaylistView::on_file_changed(std::filesystem::path new_file_path, rf::Frequency replay_frequency, uint32_t replay_sample_rate, uint32_t next_delay) {
     File data_file;
+
     // Get file size
-    auto data_open_error = data_file.open("/" + new_file_path.string());
-    if (!data_open_error.is_valid()) {
-        track_number = track_number >= total_tracks ? 1 : track_number + 1;  // prevent track_number out of range
-    } else if (data_open_error.is_valid()) {
+    auto error = data_file.open("/" + new_file_path.string());
+
+    if (error) {
         file_error("C16 file\n" + new_file_path.string() + "\nread error.");
         return;
     }
 
+    track_number = track_number >= total_tracks ? 1 : track_number + 1;  // prevent track_number out of range
+
     file_path = new_file_path;
     field_frequency.set_value(replay_frequency);
 
@@ -290,7 +267,7 @@ PlaylistView::PlaylistView(
         &tx_view,  // this handles now the previous rfgain, rfamp
         &check_loop,
         &button_play,
-        &text_track,  // removed because there's no space for it
+        &text_track,
         &waterfall,
     });
 

firmware/application/apps/ui_playlist.hpp

@@ -77,7 +77,6 @@ class PlaylistView : public View {
     const size_t buffer_count{3};
 
     void load_file(std::filesystem::path playlist_path);
-    void txtline_process(std::string&);
     void on_file_changed(std::filesystem::path new_file_path, rf::Frequency replay_frequency, uint32_t replay_sample_rate, uint32_t next_delay);
     void on_tx_progress(const uint32_t progress);
     void set_target_frequency(const rf::Frequency new_value);

firmware/application/file_reader.cpp

@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2023 Kyle Reed
+ *
+ * This file is part of PortaPack.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include "file_reader.hpp"
+#include <string_view>
+#include <vector>
+
+/* Splits the string on the specified char and returns
+ * a vector of string_views. NB: the lifetime of the
+ * string to split must be maintained while the views
+ * are used or they will dangle. */
+std::vector<std::string_view> split_string(std::string_view str, char c) {
+    std::vector<std::string_view> cols;
+    size_t start = 0;
+
+    while (start < str.length()) {
+        auto it = str.find(c, start);
+
+        if (it == str.npos)
+            break;
+
+        cols.emplace_back(&str[start], it - start);
+        start = it + 1;
+    }
+
+    if (start <= str.length() && !str.empty())
+        cols.emplace_back(&str[start], str.length() - start);
+
+    return cols;
+}

firmware/application/file_reader.hpp

@@ -131,25 +131,6 @@ using FileLineReader = BufferLineReader<File>;
  * a vector of string_views. NB: the lifetime of the
  * string to split must be maintained while the views
  * are used or they will dangle. */
-std::vector<std::string_view> split_string(std::string_view str, char c) {
-    std::vector<std::string_view> cols;
-    size_t start = 0;
-
-    while (start < str.length()) {
-        auto it = str.find(c, start);
-
-        if (it == str.npos)
-            break;
-
-        // TODO: allow empty?
-        cols.emplace_back(&str[start], it - start);
-        start = it + 1;
-    }
-
-    if (start <= str.length() && !str.empty())
-        cols.emplace_back(&str[start], str.length() - start);
-
-    return cols;
-}
+std::vector<std::string_view> split_string(std::string_view str, char c);
 
 #endif

firmware/application/string_format.cpp

@@ -283,17 +283,17 @@ double get_decimals(double num, int16_t mult, bool round) {
 
 static const char* whitespace_str = " \t\r\n";
 
-std::string trim(const std::string& str) {
+std::string trim(std::string_view str) {
     auto first = str.find_first_not_of(whitespace_str);
     auto last = str.find_last_not_of(whitespace_str);
-    return str.substr(first, last - first);
+    return std::string{str.substr(first, last - first)};
 }
 
-std::string trimr(const std::string& str) {
+std::string trimr(std::string_view str) {
     size_t last = str.find_last_not_of(whitespace_str);
-    return (last != std::string::npos) ? str.substr(0, last + 1) : "";  // Remove the trailing spaces
+    return std::string{last != std::string::npos ? str.substr(0, last + 1) : ""};
 }
 
-std::string truncate(const std::string& str, size_t length) {
-    return str.length() <= length ? str : str.substr(0, length);
+std::string truncate(std::string_view str, size_t length) {
+    return std::string{str.length() <= length ? str : str.substr(0, length)};
 }

firmware/application/string_format.hpp

@@ -25,6 +25,7 @@
 #include <cstdint>
 #include <array>
 #include <string>
+#include <string_view>
 
 #include "file.hpp"
 
@@ -66,10 +67,10 @@ std::string to_string_FAT_timestamp(const FATTimestamp& timestamp);
 std::string to_string_file_size(uint32_t file_size);
 
 std::string unit_auto_scale(double n, const uint32_t base_nano, uint32_t precision);
-double get_decimals(double num, int16_t mult, bool round = false);  // euquiq added
+double get_decimals(double num, int16_t mult, bool round = false);
 
-std::string trim(const std::string& str);   // Remove whitespace at ends.
-std::string trimr(const std::string& str);  // Remove trailing spaces
-std::string truncate(const std::string& str, size_t length);
+std::string trim(std::string_view str);   // Remove whitespace at ends.
+std::string trimr(std::string_view str);  // Remove trailing spaces
+std::string truncate(std::string_view, size_t length);
 
 #endif /*__STRING_FORMAT_H__*/

firmware/common/convert.hpp

@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2023 Kyle Reed
+ *
+ * This file is part of PortaPack.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#ifndef __CONVERT_H__
+#define __CONVERT_H__
+
+#include <charconv>
+#include <cstdlib>
+#include <string>
+#include <string_view>
+#include <type_traits>
+
+/* Zero-allocation conversion helper. */
+/* Notes:
+ * - T must be an integer type.
+ * - For base 16, input _must not_ contain a '0x' or '0X' prefix.
+ * - For base 8 a leading 0 on the literal is allowed.
+ * - Leading whitespce will cause the parse to fail.
+ */
+// TODO: from_chars seems to cause code bloat.
+// Look into using strtol and friends instead.
+
+template <typename T>
+std::enable_if_t<std::is_integral_v<T>, bool> parse_int(std::string_view str, T& out_val, int base = 10) {
+    auto result = std::from_chars(str.data(), str.data() + str.length(), out_val, base);
+    return static_cast<int>(result.ec) == 0;
+}
+
+#endif /*__CONVERT_H__*/

firmware/test/application/CMakeLists.txt

@@ -36,11 +36,14 @@ add_executable(application_test EXCLUDE_FROM_ALL
 	${PROJECT_SOURCE_DIR}/main.cpp
 	${PROJECT_SOURCE_DIR}/test_basics.cpp
 	${PROJECT_SOURCE_DIR}/test_circular_buffer.cpp
+	${PROJECT_SOURCE_DIR}/test_convert.cpp
 	${PROJECT_SOURCE_DIR}/test_file_reader.cpp
 	${PROJECT_SOURCE_DIR}/test_file_wrapper.cpp
 	${PROJECT_SOURCE_DIR}/test_mock_file.cpp
 	${PROJECT_SOURCE_DIR}/test_optional.cpp
 	${PROJECT_SOURCE_DIR}/test_utility.cpp
+
+	${PROJECT_SOURCE_DIR}/../../application/file_reader.cpp
 )
 
 target_include_directories(application_test PRIVATE

firmware/test/application/test_convert.cpp

@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2023
+ *
+ * This file is part of PortaPack.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include "doctest.h"
+#include "convert.hpp"
+#include <string>
+#include <string_view>
+
+TEST_SUITE_BEGIN("parse_int");
+
+TEST_CASE("It should convert literals.") {
+    int val = 0;
+    REQUIRE(parse_int("12345", val));
+    CHECK_EQ(val, 12345);
+}
+
+TEST_CASE("It should convert strings.") {
+    int val = 0;
+    std::string s = "12345";
+    REQUIRE(parse_int(s, val));
+    CHECK_EQ(val, 12345);
+}
+
+TEST_CASE("It should convert string_views.") {
+    int val = 0;
+    std::string_view s{"12345"};
+    REQUIRE(parse_int(s, val));
+    CHECK_EQ(val, 12345);
+}
+
+TEST_CASE("It should return false for invalid input") {
+    int val = 0;
+    REQUIRE_FALSE(parse_int("QWERTY", val));
+}
+
+TEST_CASE("It should return false for overflow input") {
+    uint8_t val = 0;
+    REQUIRE_FALSE(parse_int("500", val));
+}
+
+TEST_CASE("It should convert base 16.") {
+    int val = 0;
+    REQUIRE(parse_int("30", val, 16));  // NB: No '0x'
+    CHECK_EQ(val, 0x30);
+}
+
+TEST_CASE("It should convert base 8.") {
+    int val = 0;
+    REQUIRE(parse_int("020", val, 8));
+    CHECK_EQ(val, 020);
+}
+
+TEST_CASE("It should convert as much of the string as it can.") {
+    int val = 0;
+    REQUIRE(parse_int("12345foobar", val));
+    CHECK_EQ(val, 12345);
+}
+
+TEST_SUITE_END();