Merge pull request #1201 from FarmBot/qa/10.0.0

FBOS v10.0.0

.circleci/config.yml

@@ -2,7 +2,7 @@ version: 2.0
 defaults: &defaults
   working_directory: /nerves/build
   docker:
-    - image: nervesproject/nerves_system_br:latest
+    - image: nervesproject/nerves_system_br:1.11.3
 
 install_elixir: &install_elixir
   run:

CHANGELOG.md

@@ -5,6 +5,7 @@
  * Deprecate `resource_update` RPC
  * Introduce `update_resource` RPC, which allows users to modify variables from the sequence editor.
  * Genesis v1.5 and Express v1.0 firmware updates.
+ * Fix a bug where FBOS would not honor an "AUTO UPDATE" value of "false".
 
 # 9.2.2
 

VERSION

@@ -1 +1 @@
-10.0.0-rc29
+10.0.0-rc47

farmbot_celery_script/lib/farmbot_celery_script/compilers/update_resource_compiler.ex

@@ -6,8 +6,8 @@ defmodule FarmbotCeleryScript.Compiler.UpdateResource do
       me = unquote(__MODULE__)
       variable = unquote(Map.fetch!(args, :resource))
       update = unquote(unpair(body, %{}))
+
       # Go easy on the API...
-      Process.sleep(1234)
       case variable do
         %AST{kind: :identifier} ->
           args = Map.fetch!(variable, :args)
@@ -49,7 +49,6 @@ defmodule FarmbotCeleryScript.Compiler.UpdateResource do
   end
 
   defp unpair([pair | rest], acc) do
-    IO.puts("TODO: Need to apply handlebars to `value`s.")
     key = Map.fetch!(pair.args, :label)
     val = Map.fetch!(pair.args, :value)
     next_acc = Map.merge(acc, DotProps.create(key, val))

farmbot_celery_script/test/farmbot_celery_script/compiler_groups_test.exs

@@ -47,8 +47,8 @@ defmodule FarmbotCeleryScript.CompilerGroupsTest do
     canary_actual = :crypto.hash(:sha, Macro.to_string(result))
 
     canary_expected =
-      <<157, 69, 5, 38, 188, 78, 10, 183, 154, 99, 151, 193, 214, 208, 187, 130,
+      <<136, 140, 48, 226, 216, 155, 178, 103, 244, 88, 225, 146, 130, 216, 125,
-        183, 73, 13, 48>>
+        72, 113, 195, 65, 1>>
 
     # READ THE NOTE ABOVE IF THIS TEST FAILS!!!
     assert canary_expected == canary_actual

farmbot_celery_script/test/farmbot_celery_script/compiler_test.exs

@@ -79,7 +79,7 @@ defmodule FarmbotCeleryScript.CompilerTest do
   end
 
   test "identifier sanitization" do
-    label = "System.cmd(\"rm\", [\"-rf /*\"])"
+    label = "System.cmd(\"echo\", [\"lol\"])"
     value_ast = AST.Factory.new("coordinate", x: 1, y: 1, z: 1)
     identifier_ast = AST.Factory.new("identifier", label: label)
 
@@ -120,11 +120,19 @@ defmodule FarmbotCeleryScript.CompilerTest do
              [
                fn params ->
                  _ = inspect(params)
+                 unsafe_U3lzdGVtLmNtZCgiZWNobyIsIFsibG9sIl0p = FarmbotCeleryScript.SysCalls.coordinate(1, 1, 1)
 
-                 #{var_name} =
+                 better_params = %{
-                   FarmbotCeleryScript.SysCalls.coordinate(1, 1, 1)
+                   "System.cmd(\\"echo\\", [\\"lol\\"])" => %FarmbotCeleryScript.AST{
+                     args: %{x: 1, y: 1, z: 1},
+                     body: [],
+                     comment: nil,
+                     kind: :coordinate,
+                     meta: nil
+                   }
+                 }
 
-                 [fn -> #{var_name} end]
+                 [fn -> unsafe_U3lzdGVtLmNtZCgiZWNobyIsIFsibG9sIl0p end]
                end
              ]
              """)
@@ -372,19 +380,38 @@ defmodule FarmbotCeleryScript.CompilerTest do
                  unsafe_cGFyZW50 =
                    Keyword.get(params, :unsafe_cGFyZW50, FarmbotCeleryScript.SysCalls.coordinate(1, 2, 3))
 
+                 better_params = %{}
+
                  [
                    fn ->
-                     FarmbotCeleryScript.Compiler.UpdateResource.do_update(
+                     me = FarmbotCeleryScript.Compiler.UpdateResource
-                       %FarmbotCeleryScript.AST{
+
-                         args: %{label: "parent"},
+                     variable = %FarmbotCeleryScript.AST{
-                         body: [],
+                       args: %{label: "parent"},
-                         comment: nil,
+                       body: [],
-                         kind: :identifier,
+                       comment: nil,
-                         meta: nil
+                       kind: :identifier,
-                       },
+                       meta: nil
-                       %{"plant_stage" => "removed"},
+                     }
-                       []
+
-                     )
+                     update = %{"plant_stage" => "removed"}
+
+                     case(variable) do
+                       %AST{kind: :identifier} ->
+                         args = Map.fetch!(variable, :args)
+                         label = Map.fetch!(args, :label)
+                         resource = Map.fetch!(better_params, label)
+                         me.do_update(resource, update)
+
+                       %AST{kind: :point} ->
+                         me.do_update(variable.args(), update)
+
+                       %AST{kind: :resource} ->
+                         me.do_update(variable.args(), update)
+
+                       res ->
+                         raise("Resource error. Please notfiy support: \#{inspect(res)}")
+                     end
                    end
                  ]
                end
@@ -405,20 +432,38 @@ defmodule FarmbotCeleryScript.CompilerTest do
              [
                fn params ->
                  _ = inspect(params)
+                 better_params = %{}
 
                  [
                    fn ->
-                     FarmbotCeleryScript.Compiler.UpdateResource.do_update(
+                     me = FarmbotCeleryScript.Compiler.UpdateResource
-                       %FarmbotCeleryScript.AST{
+
-                         args: %{resource_id: 23, resource_type: "Plant"},
+                     variable = %FarmbotCeleryScript.AST{
-                         body: [],
+                       args: %{resource_id: 23, resource_type: "Plant"},
-                         comment: nil,
+                       body: [],
-                         kind: :resource,
+                       comment: nil,
-                         meta: nil
+                       kind: :resource,
-                       },
+                       meta: nil
-                       %{"plant_stage" => "planted", "r" => 23},
+                     }
-                       []
+
-                     )
+                     update = %{"plant_stage" => "planted", "r" => 23}
+
+                     case(variable) do
+                       %AST{kind: :identifier} ->
+                         args = Map.fetch!(variable, :args)
+                         label = Map.fetch!(args, :label)
+                         resource = Map.fetch!(better_params, label)
+                         me.do_update(resource, update)
+
+                       %AST{kind: :point} ->
+                         me.do_update(variable.args(), update)
+
+                       %AST{kind: :resource} ->
+                         me.do_update(variable.args(), update)
+
+                       res ->
+                         raise("Resource error. Please notfiy support: \#{inspect(res)}")
+                     end
                    end
                  ]
                end

farmbot_core/lib/farmbot_core/asset.ex

@@ -6,15 +6,15 @@ defmodule FarmbotCore.Asset do
   """
 
   alias FarmbotCore.Asset.{
-    Repo,
+    CriteriaRetriever,
     Device,
     DeviceCert,
-    FarmwareEnv,
-    FirstPartyFarmware,
-    FarmwareInstallation,
     FarmEvent,
+    FarmwareEnv,
+    FarmwareInstallation,
     FbosConfig,
     FirmwareConfig,
+    FirstPartyFarmware,
     Peripheral,
     PinBinding,
     Point,
@@ -22,11 +22,11 @@ defmodule FarmbotCore.Asset do
     PublicKey,
     Regimen,
     RegimenInstance,
-    Sequence,
+    Repo,
     Sensor,
     SensorReading,
+    Sequence,
     Tool,
-    CriteriaRetriever
   }
 
   alias FarmbotCore.AssetSupervisor
@@ -251,7 +251,7 @@ defmodule FarmbotCore.Asset do
   end
 
   def update_point(point, params) do
-    # TODO: RC 8 MAY 20202 - We need to hard refresh the point.
+    # TODO: RC 8 MAY 2020 - We need to hard refresh the point.
     #       The CSVM appears to be caching resources. This leads
     #       to problems when a user runs a sequence that has two
     #       MARK AS steps.

farmbot_core/lib/farmbot_core/asset/private.ex

@@ -45,9 +45,7 @@ defmodule FarmbotCore.Asset.Private do
     # error being thrown.
     changeset = LocalMeta.changeset(local_meta, Map.merge(params, %{table: table, status: "dirty"}))
     try do
-      result =  Repo.insert_or_update!(changeset)
+      Repo.insert_or_update!(changeset)
-      %FarmbotCore.Asset.Private.LocalMeta{} = result
-      result
     catch
       :error,  %Sqlite.DbConnection.Error{
         message: "UNIQUE constraint failed: local_metas.table, local_metas.asset_local_id",

farmbot_ext/lib/farmbot_ext/api/dirty_worker.ex

@@ -6,8 +6,9 @@ defmodule FarmbotExt.API.DirtyWorker do
   import API.View, only: [render: 2]
 
   require Logger
+  require FarmbotCore.Logger
   use GenServer
-  @timeout 4700
+  @timeout 500
 
   # these resources can't be accessed by `id`.
   @singular [
@@ -35,66 +36,68 @@ defmodule FarmbotExt.API.DirtyWorker do
   @impl GenServer
   def init(args) do
     module = Keyword.fetch!(args, :module)
-    timeout = Keyword.get(args, :timeout, @timeout)
+    Process.send_after(self(), :do_work, @timeout)
-    timer = Process.send_after(self(), :timeout, timeout)
+    {:ok, %{module: module}}
-    {:ok, %{module: module, timeout: timeout, timer: timer}}
   end
 
   @impl GenServer
-  def handle_info(:timeout, %{module: module} = state) do
+  def handle_info(:do_work, %{module: module} = state) do
-    dirty = Private.list_dirty(module)
+    Process.sleep(@timeout)
-    local = Private.list_local(module)
+    list = Enum.uniq(Private.list_dirty(module) ++ Private.list_local(module))
-    {:noreply, state, {:continue, Enum.uniq(dirty ++ local)}}
+
+    unless has_race_condition?(module, list) do
+      Enum.map(list, fn dirty -> work(dirty, module) end)
+    end
+
+    Process.send_after(self(), :do_work, @timeout)
+    {:noreply, state}
   end
 
-  @impl GenServer
+  def work(dirty, module) do
-  def handle_continue([], state) do
+    # Go easy on the API
-    timer = Process.send_after(self(), :timeout, state.timeout)
+    Process.sleep(333)
-    {:noreply, %{state | timer: timer}}
-  end
 
-  def handle_continue([dirty | rest], %{module: module} = state) do
+    case http_request(dirty, module) do
-    case http_request(dirty, state) do
       # Valid data
       {:ok, %{status: s, body: body}} when s > 199 and s < 300 ->
-        dirty |> module.changeset(body) |> handle_changeset(rest, state)
+        dirty |> module.changeset(body) |> handle_changeset(module)
 
       # Invalid data
       {:ok, %{status: s, body: %{} = body}} when s > 399 and s < 500 ->
+        FarmbotCore.Logger.error(2, "HTTP Error #{s}. #{inspect(body)}")
         changeset = module.changeset(dirty)
 
         Enum.reduce(body, changeset, fn {key, val}, changeset ->
           Ecto.Changeset.add_error(changeset, key, val)
         end)
-        |> handle_changeset(rest, state)
+        |> handle_changeset(module)
 
       # Invalid data, but the API didn't say why
       {:ok, %{status: s, body: _body}} when s > 399 and s < 500 ->
+        FarmbotCore.Logger.error(2, "HTTP Error #{s}. #{inspect(dirty)}")
+
         module.changeset(dirty)
         |> Map.put(:valid?, false)
-        |> handle_changeset(rest, state)
+        |> handle_changeset(module)
 
       # HTTP Error. (500, network error, timeout etc.)
       error ->
-        Logger.error(
+        FarmbotCore.Logger.error(
-          "[#{module} #{dirty.local_id} #{inspect(self())}] HTTP Error: #{state.module} #{
+          2,
+          "[#{module} #{dirty.local_id} #{inspect(self())}] HTTP Error: #{module} #{
             inspect(error)
           }"
         )
-
-        {:noreply, state, @timeout}
     end
   end
 
   # If the changeset was valid, update the record.
-  def handle_changeset(%{valid?: true} = changeset, rest, state) do
+  def handle_changeset(%{valid?: true} = changeset, _module) do
-    Repo.update!(changeset)
+    Private.mark_clean!(Repo.update!(changeset))
-    |> Private.mark_clean!()
+    :ok
-
-    {:noreply, state, {:continue, rest}}
   end
 
-  def handle_changeset(%{valid?: false, data: data} = changeset, rest, state) do
+  def handle_changeset(%{valid?: false, data: data} = changeset, module) do
     message =
       Enum.map(changeset.errors, fn
         {key, {msg, _meta}} when is_binary(key) -> "\t#{key}: #{msg}"
@@ -102,26 +105,64 @@ defmodule FarmbotExt.API.DirtyWorker do
       end)
       |> Enum.join("\n")
 
-    Logger.error("Failed to sync: #{state.module} \n #{message}")
+    FarmbotCore.Logger.error(3, "Failed to sync: #{module} \n #{message}")
     _ = Repo.delete!(data)
-    {:noreply, state, {:continue, rest}}
+    :ok
   end
 
-  defp http_request(%{id: nil} = dirty, state) do
+  defp http_request(%{id: nil} = dirty, module) do
-    path = state.module.path()
+    path = module.path()
-    data = render(state.module, dirty)
+    data = render(module, dirty)
     API.post(API.client(), path, data)
   end
 
-  defp http_request(dirty, %{module: module} = state) when module in @singular do
+  defp http_request(dirty, module) when module in @singular do
-    path = path = state.module.path()
+    path = path = module.path()
-    data = render(state.module, dirty)
+    data = render(module, dirty)
     API.patch(API.client(), path, data)
   end
 
-  defp http_request(dirty, state) do
+  defp http_request(dirty, module) do
-    path = Path.join(state.module.path(), to_string(dirty.id))
+    path = Path.join(module.path(), to_string(dirty.id))
-    data = render(state.module, dirty)
+    data = render(module, dirty)
     API.patch(API.client(), path, data)
   end
+
+  # This is a fix for a race condtion. The root cause is unknown
+  # as of 18 May 2020. The problem is that records are marked
+  # diry _before_ the dirty data is saved. That means that FBOS
+  # knows a record has changed, but for a brief moment, it only
+  # has the old copy of the record (not the changes).
+  # Because of this race condtion,
+  # The race condition:
+  #
+  # * Is nondeterministic
+  # * Happens frequently when running many MARK AS steps in one go.
+  # * Happens frequently when Erlang VM only has one thread
+  #    * Ie: `iex --erl '+S 1 +A 1' -S mix`
+  # * Happens frequently when @timeout is decreased to `1`.
+  #
+  # This function PREVENTS CORRUPTION OF API DATA. It can be
+  # removed once the root cause of the data race is determined.
+  #   - RC 18 May 2020
+  def has_race_condition?(module, list) do
+    Enum.find(list, fn item ->
+      if item.id do
+        if item == Repo.get_by(module, id: item.id) do
+          # This item is OK - no race condition.
+          false
+        else
+          # There was a race condtion. We probably can't trust
+          # any of the data in this list. We need to wait and
+          # try again later.
+          Process.sleep(@timeout * 3)
+          true
+        end
+      else
+        # This item only exists on the FBOS side.
+        # It will never be affected by the data race condtion.
+        false
+      end
+    end)
+  end
 end

farmbot_os/lib/farmbot_os/sys_calls/resource_update.ex

@@ -32,11 +32,11 @@ defmodule FarmbotOS.SysCalls.ResourceUpdate do
     "Weed" => "weed"
   }
 
-  def notify_user_of_updates(kind, params) do
+  def notify_user_of_updates(kind, params, id \\ nil) do
     Enum.map(params, fn {k, v} ->
       name = @friendly_names[kind] || kind
       property = @friendly_names["#{k}"] || k
-      msg = "Setting #{name} #{property} to #{inspect(v)}"
+      msg = "Setting #{name} #{id} #{property} to #{inspect(v)}"
       FarmbotCore.Logger.info(3, msg)
     end)
   end
@@ -53,7 +53,7 @@ defmodule FarmbotOS.SysCalls.ResourceUpdate do
   end
 
   def update_resource(kind, id, params) when kind in @point_kinds do
-    notify_user_of_updates(kind, params)
+    notify_user_of_updates(kind, params, id)
     params = do_handlebars(params)
     point_update_resource(kind, id, params)
   end

farmbot_os/platform/target/nerves_hub_client.ex

@@ -542,7 +542,7 @@ defmodule FarmbotOS.Platform.Target.NervesHubClient do
           "ota_hour = #{ota_hour || "null"} timezone = #{timezone || "null"}"
         )
 
-        true
+        !!auto_update
       end
 
     result && !currently_downloading?()