Merge pull request #130 from ianfixes/2019-07-31_bugfixes

Bugfixes

Author: ianfixes

CHANGELOG.md

@@ -7,35 +7,38 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 ### Added
-
 - Minimal Wire mocks. Will not provide support for unit testing I2C communication yet, but will allow compilation of libraries that use I2C.
+- `StreamTape` class now bridges `Stream` and `HardwareSerial` to allow general-purpose stream mocking & history
 
 ### Changed
+- Arduino command failures (to read preferences) now causes a fatal error, with help for troubleshooting the underlying command
 
 ### Deprecated
 
 ### Removed
 
 ### Fixed
+- Arduino library dependencies are now installed prior to unit testing, instead of prior to compilation testing.  Whoops.
+- Arduino library dependencies with spaces in their names are now handled properly during compilation -- spaces are automatically coerced to underscores
 
 ### Security
 
 
 ## [0.2.0] - 2019-02-20
 ### Added
-* `release-new-version.sh` script
-* outputs for `PinHistory` can now report timestamps
-* Fibonacci Clock for clock testing purposes (internal to this library)
+- `release-new-version.sh` script
+- outputs for `PinHistory` can now report timestamps
+- Fibonacci Clock for clock testing purposes (internal to this library)
 
 ### Changed
-* Shortened `ArduinoQueue` push and pop operations
-* `ci/Queue.h` is now `MockEventQueue.h`, with timing data
-* `MockEventQueue::Node` now contains struct `MockEventQueue::Event`, which contains both the templated type `T` and a field for a timestamp.
-* Construction of `MockEventQueue` now includes a constructor argument for the time-fetching function
-* Construction of `PinHistory` now includes a constructor argument for the time-fetching function
-* `PinHistory` can now return an array of timestamps for its events
-* `GodmodeState` is now a singleton pattern, which is necessary to support the globality of Arduino functions
-* `GodmodeState` now uses timestamped PinHistory for Analog and Digital
+- Shortened `ArduinoQueue` push and pop operations
+- `ci/Queue.h` is now `MockEventQueue.h`, with timing data
+- `MockEventQueue::Node` now contains struct `MockEventQueue::Event`, which contains both the templated type `T` and a field for a timestamp.
+- Construction of `MockEventQueue` now includes a constructor argument for the time-fetching function
+- Construction of `PinHistory` now includes a constructor argument for the time-fetching function
+- `PinHistory` can now return an array of timestamps for its events
+- `GodmodeState` is now a singleton pattern, which is necessary to support the globality of Arduino functions
+- `GodmodeState` now uses timestamped PinHistory for Analog and Digital
 
 ### Fixed
 * `ArduinoQueue` no longer leaks memory

cpp/arduino/HardwareSerial.h

@@ -1,7 +1,7 @@
 #pragma once
 
 //#include <inttypes.h>
-#include "Stream.h"
+#include "ci/StreamTape.h"
 
 // definitions neeeded for Serial.begin's config arg
 #define SERIAL_5N1 0x00
@@ -29,38 +29,19 @@
 #define SERIAL_7O2 0x3C
 #define SERIAL_8O2 0x3E
 
-class HardwareSerial : public Stream, public ObservableDataStream
+class HardwareSerial : public StreamTape
 {
-  protected:
-    String* mGodmodeDataOut;
-
   public:
-    HardwareSerial(String* dataIn, String* dataOut, unsigned long* delay): Stream(), ObservableDataStream() {
-      mGodmodeDataIn      = dataIn;
-      mGodmodeDataOut     = dataOut;
-      mGodmodeMicrosDelay = delay;
-    }
+    HardwareSerial(String* dataIn, String* dataOut, unsigned long* delay): StreamTape(dataIn, dataOut, delay) {}
+
     void begin(unsigned long baud) { begin(baud, SERIAL_8N1); }
     void begin(unsigned long baud, uint8_t config) {
       *mGodmodeMicrosDelay = 1000000 / baud;
     }
     void end() {}
 
-    // virtual int available(void);
-    // virtual int peek(void);
-    // virtual int read(void);
-    // virtual int availableForWrite(void);
-    // virtual void flush(void);
-    virtual size_t write(uint8_t aChar) {
-      mGodmodeDataOut->append(String((char)aChar));
-      advertiseByte((unsigned char)aChar);
-      return 1;
-    }
-
-    // https://stackoverflow.com/a/4271276
-    using Print::write; // pull in write(str) and write(buf, size) from Print
+    // support "if (Serial1) {}" sorts of things
     operator bool() { return true; }
-
 };
 
 #if defined(UBRRH) || defined(UBRR0H)

cpp/arduino/ci/StreamTape.h

@@ -0,0 +1,36 @@
+#pragma once
+
+#include "../Stream.h"
+
+/**
+ * Stream with godmode-controlled input and godmode-persisted output
+ */
+class StreamTape : public Stream, public ObservableDataStream
+{
+  protected:
+    String* mGodmodeDataOut;
+    // mGodmodeDataIn is provided by Stream
+
+  public:
+    StreamTape(String* dataIn, String* dataOut, unsigned long* delay): Stream(), ObservableDataStream() {
+      mGodmodeDataIn      = dataIn;
+      mGodmodeDataOut     = dataOut;
+      mGodmodeMicrosDelay = delay;
+    }
+
+    // virtual int available(void);
+    // virtual int peek(void);
+    // virtual int read(void);
+    // virtual int availableForWrite(void);
+    // virtual void flush(void);
+    virtual size_t write(uint8_t aChar) {
+      mGodmodeDataOut->append(String((char)aChar));
+      advertiseByte((unsigned char)aChar);
+      return 1;
+    }
+
+    // https://stackoverflow.com/a/4271276
+    using Print::write; // pull in write(str) and write(buf, size) from Print
+
+};
+

exe/arduino_ci_remote.rb

@@ -167,6 +167,16 @@ def display_files(pathname)
   non_hidden.each { |p| puts "#{margin}#{p}" }
 end
 
+def install_arduino_library_dependencies(aux_libraries)
+  aux_libraries.each do |l|
+    if @arduino_cmd.library_present?(l)
+      inform("Using pre-existing library") { l.to_s }
+    else
+      assure("Installing aux library '#{l}'") { @arduino_cmd.install_library(l) }
+    end
+  end
+end
+
 def perform_unit_tests(file_config)
   if @cli_options[:skip_unittests]
     inform("Skipping unit tests") { "as requested via command line" }
@@ -209,6 +219,8 @@ def perform_unit_tests(file_config)
   elsif config.platforms_to_unittest.empty?
     inform("Skipping unit tests") { "no platforms were requested" }
   else
+    install_arduino_library_dependencies(config.aux_libraries_for_unittest)
+
     config.platforms_to_unittest.each do |p|
       config.allowable_unittest_files(cpp_library.test_files).each do |unittest_path|
         unittest_name = unittest_path.basename.to_s
@@ -273,7 +285,7 @@ def perform_compilation_tests(config)
   # while we're doing that, get the aux libraries as well
   example_platform_info = {}
   board_package_url = {}
-  aux_libraries = Set.new(config.aux_libraries_for_unittest + config.aux_libraries_for_build)
+  aux_libraries = Set.new(config.aux_libraries_for_build)
   # while collecting the platforms, ensure they're defined
 
   library_examples.each do |path|
@@ -322,13 +334,7 @@ def perform_compilation_tests(config)
     end
   end
 
-  aux_libraries.each do |l|
-    if @arduino_cmd.library_present?(l)
-      inform("Using pre-existing library") { l.to_s }
-    else
-      assure("Installing aux library '#{l}'") { @arduino_cmd.install_library(l) }
-    end
-  end
+  install_arduino_library_dependencies(aux_libraries)
 
   last_board = nil
   if config.platforms_to_build.empty?

lib/arduino_ci/arduino_cmd.rb

@@ -6,6 +6,9 @@
 
 module ArduinoCI
 
+  # To report errors that we can't resolve or possibly even explain
+  class ArduinoExecutionError < StandardError; end
+
   # Wrap the Arduino executable.  This requires, in some cases, a faked display.
   class ArduinoCmd
 
@@ -42,14 +45,14 @@ def self.flag(name, text = nil)
     attr_reader   :last_msg
 
     # set the command line flags (undefined for now).
-    # These vary between gui/cli
-    flag :get_pref
-    flag :set_pref
-    flag :save_prefs
-    flag :use_board
-    flag :install_boards
-    flag :install_library
-    flag :verify
+    # These vary between gui/cli.  Inline comments added for greppability
+    flag :get_pref           # flag_get_pref
+    flag :set_pref           # flag_set_pref
+    flag :save_prefs         # flag_save_prefs
+    flag :use_board          # flag_use_board
+    flag :install_boards     # flag_install_boards
+    flag :install_library    # flag_install_library
+    flag :verify             # flag_verify
 
     def initialize
       @prefs_cache        = {}
@@ -82,7 +85,8 @@ def lib_dir
     # @return [String] Preferences as a set of lines
     def _prefs_raw
       resp = run_and_capture(flag_get_pref)
-      return nil unless resp[:success]
+      fail_msg = "Arduino binary failed to operate as expected; you will have to troubleshoot it manually"
+      raise ArduinoExecutionError, "#{fail_msg}. The command was #{@last_msg}" unless resp[:success]
 
       @prefs_fetched = true
       resp[:out]

lib/arduino_ci/ci_config.rb

@@ -293,9 +293,12 @@ def allowable_unittest_files(paths)
       return paths if @unittest_info[:testfiles].nil?
 
       ret = paths
+      # Check for array emptiness, otherwise nothing will be selected!
       unless @unittest_info[:testfiles][:select].nil? || @unittest_info[:testfiles][:select].empty?
         ret.select! { |p| unittest_info[:testfiles][:select].any? { |glob| p.basename.fnmatch(glob) } }
       end
+
+      # It's OK for the :reject array to be empty, that means nothing will be rejected by default
       unless @unittest_info[:testfiles][:reject].nil?
         ret.reject! { |p| unittest_info[:testfiles][:reject].any? { |glob| p.basename.fnmatch(glob) } }
       end

lib/arduino_ci/cpp_library.rb

@@ -220,7 +220,10 @@ def arduino_library_src_dirs(aux_libraries)
       # TODO: be smart and implement library spec (library.properties, etc)?
       subdirs = ["", "src", "utility"]
       all_aux_include_dirs_nested = aux_libraries.map do |libdir|
-        subdirs.map { |subdir| Pathname.new(@arduino_lib_dir) + libdir + subdir }
+        # library manager coerces spaces in package names to underscores
+        # see https://github.com/ianfixes/arduino_ci/issues/132#issuecomment-518857059
+        legal_libdir = libdir.tr(" ", "_")
+        subdirs.map { |subdir| Pathname.new(@arduino_lib_dir) + legal_libdir + subdir }
       end
       all_aux_include_dirs_nested.flatten.select(&:exist?).select(&:directory?)
     end