Merge branch 'reboot'

benma · benma · commit 8ffbb5e7d0a5 · 2021-04-23T15:59:32.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## [Unreleased]
 - Attempt to fix flaky SD behavior
 - Add securechip_model to DeviceInfo: ATECCC608A or ATECC608B.
+- Added reboot purpose for clearer UX: "Proceed to upgrade?" vs. "Go to startup settings?"
 
 ## 9.5.0 [released 2021-03-10]
 - RestoreFrommnemonic: ported to Rust. Will now return UserAbortError on user abort instead of GenericError.
diff --git a/messages/system.proto b/messages/system.proto
@@ -16,5 +16,9 @@ syntax = "proto3";
 package shiftcrypto.bitbox02;
 
 message RebootRequest {
-
+  enum Purpose {
+    UPGRADE = 0;
+    SETTINGS = 1;
+  }
+  Purpose purpose = 1;
 }
diff --git a/py/bitbox02/bitbox02/bitbox02/__init__.py b/py/bitbox02/bitbox02/bitbox02/__init__.py
@@ -54,5 +54,6 @@
     btc,
     common,
     eth,
+    system,
 )
 from .bootloader import Bootloader
diff --git a/py/bitbox02/bitbox02/bitbox02/bitbox02.py b/py/bitbox02/bitbox02/bitbox02/bitbox02.py
@@ -44,6 +44,10 @@
     from bitbox02.communication.generated import common_pb2 as common
     from bitbox02.communication.generated import keystore_pb2 as keystore
     from bitbox02.communication.generated import antiklepto_pb2 as antiklepto
+
+    # pylint: disable=unused-import
+    # We export it in __init__.py
+    from bitbox02.communication.generated import system_pb2 as system
 except ModuleNotFoundError:
     print("Run `make py` to generate the protobuf messages")
     sys.exit()
diff --git a/py/bitbox02/bitbox02/communication/bitbox_api_protocol.py b/py/bitbox02/bitbox02/communication/bitbox_api_protocol.py
@@ -647,11 +647,17 @@ def _msg_query(
             print(response)
         return response
 
-    def reboot(self) -> bool:
-        """TODO: Document"""
+    def reboot(
+        self, purpose: system.RebootRequest.Purpose = system.RebootRequest.Purpose.UPGRADE
+    ) -> bool:
+        """
+        Sends the reboot request. If the user confirms the request on the device, the device reboots
+        into the bootloader.
+        The purpose defines what confirmation message the user gets to see on the device.
+        """
         # pylint: disable=no-member
         request = hww.Request()
-        request.reboot.CopyFrom(system.RebootRequest())
+        request.reboot.CopyFrom(system.RebootRequest(purpose=purpose))
         try:
             self._msg_query(request)
         except OSError:
diff --git a/py/bitbox02/bitbox02/communication/generated/system_pb2.py b/py/bitbox02/bitbox02/communication/generated/system_pb2.py
diff --git a/py/bitbox02/bitbox02/communication/generated/system_pb2.pyi b/py/bitbox02/bitbox02/communication/generated/system_pb2.pyi
@@ -1,15 +1,54 @@
 # @generated by generate_proto_mypy_stubs.py.  Do not edit!
 import sys
+from google.protobuf.descriptor import (
+    EnumDescriptor as google___protobuf___descriptor___EnumDescriptor,
+)
+
 from google.protobuf.message import (
     Message as google___protobuf___message___Message,
 )
 
+from typing import (
+    List as typing___List,
+    Optional as typing___Optional,
+    Tuple as typing___Tuple,
+    cast as typing___cast,
+)
+
+from typing_extensions import (
+    Literal as typing_extensions___Literal,
+)
+
 
 class RebootRequest(google___protobuf___message___Message):
+    class Purpose(int):
+        DESCRIPTOR: google___protobuf___descriptor___EnumDescriptor = ...
+        @classmethod
+        def Name(cls, number: int) -> str: ...
+        @classmethod
+        def Value(cls, name: str) -> RebootRequest.Purpose: ...
+        @classmethod
+        def keys(cls) -> typing___List[str]: ...
+        @classmethod
+        def values(cls) -> typing___List[RebootRequest.Purpose]: ...
+        @classmethod
+        def items(cls) -> typing___List[typing___Tuple[str, RebootRequest.Purpose]]: ...
+        UPGRADE = typing___cast(RebootRequest.Purpose, 0)
+        SETTINGS = typing___cast(RebootRequest.Purpose, 1)
+    UPGRADE = typing___cast(RebootRequest.Purpose, 0)
+    SETTINGS = typing___cast(RebootRequest.Purpose, 1)
+
+    purpose = ... # type: RebootRequest.Purpose
 
     def __init__(self,
+        *,
+        purpose : typing___Optional[RebootRequest.Purpose] = None,
         ) -> None: ...
     @classmethod
     def FromString(cls, s: bytes) -> RebootRequest: ...
     def MergeFrom(self, other_msg: google___protobuf___message___Message) -> None: ...
     def CopyFrom(self, other_msg: google___protobuf___message___Message) -> None: ...
+    if sys.version_info >= (3,):
+        def ClearField(self, field_name: typing_extensions___Literal[u"purpose"]) -> None: ...
+    else:
+        def ClearField(self, field_name: typing_extensions___Literal[u"purpose",b"purpose"]) -> None: ...
diff --git a/py/send_message.py b/py/send_message.py
@@ -241,7 +241,12 @@ def _list_device_info(self) -> None:
         print(f"All info: {self._device.device_info()}")
 
     def _reboot(self) -> None:
-        if self._device.reboot():
+        inp = input("Select one of: 1=upgrade; 2=go to startup settings: ").strip()
+        purpose = {
+            "1": bitbox02.system.RebootRequest.Purpose.UPGRADE,  # pylint: disable=no-member
+            "2": bitbox02.system.RebootRequest.Purpose.SETTINGS,  # pylint: disable=no-member
+        }[inp]
+        if self._device.reboot(purpose=purpose):
             print("Device rebooted")
             self._stop = True
         else:
@@ -577,13 +582,6 @@ def _create_backup(self) -> None:
         except UserAbortException:
             print("Aborted by user")
 
-    def _reboot_bootloader(self) -> None:
-        if self._device.reboot():
-            print("Device rebooted")
-            self._stop = True
-            return
-        print("User aborted")
-
     def _toggle_mnemonic_passphrase(self) -> None:
         enabled = self._device.device_info()["mnemonic_passphrase_enabled"]
         try:
@@ -735,7 +733,7 @@ def _menu_init(self) -> None:
             ("Check backup", self._check_backup),
             ("Show mnemonic", self._show_mnemnoic_seed),
             ("Create backup", self._create_backup),
-            ("Reboot into bootloader", self._reboot_bootloader),
+            ("Reboot into bootloader", self._reboot),
             ("Check if SD card inserted", self._check_sd_presence),
             ("Insert SD card", self._insert_sdcard),
             ("Remove SD card", self._remove_sdcard),
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -26,6 +26,7 @@ set(DBB-FIRMWARE-SOURCES
   ${CMAKE_SOURCE_DIR}/src/hardfault.c
   ${CMAKE_SOURCE_DIR}/src/util.c
   ${CMAKE_SOURCE_DIR}/src/sd.c
+  ${CMAKE_SOURCE_DIR}/src/system.c
   ${CMAKE_SOURCE_DIR}/src/hww.c
   ${CMAKE_SOURCE_DIR}/src/memory/bitbox02_smarteeprom.c
   ${CMAKE_SOURCE_DIR}/src/memory/memory.c
@@ -45,7 +46,6 @@ set(DBB-FIRMWARE-SOURCES
   ${CMAKE_SOURCE_DIR}/src/workflow/confirm_time.c
   ${CMAKE_SOURCE_DIR}/src/workflow/idle_workflow.c
   ${CMAKE_SOURCE_DIR}/src/workflow/password.c
-  ${CMAKE_SOURCE_DIR}/src/workflow/reboot.c
   ${CMAKE_SOURCE_DIR}/src/workflow/restore.c
   ${CMAKE_SOURCE_DIR}/src/workflow/orientation_screen.c
   ${CMAKE_SOURCE_DIR}/src/workflow/status.c
@@ -423,6 +423,7 @@ add_custom_target(rust-bindgen
     --whitelist-function app_eth_erc20_params_get
     --whitelist-function app_eth_sighash
     --whitelist-function app_btc_address_simple
+    --whitelist-function reboot
     ${CMAKE_CURRENT_SOURCE_DIR}/rust/bitbox02-sys/wrapper.h --
     -DPB_NO_PACKED_STRUCTS=1 -DPB_FIELD_16BIT=1 -fshort-enums ${RUST_BINDGEN_FLAGS} ${RUST_INCLUDES}
   COMMAND
diff --git a/src/commander/commander.c b/src/commander/commander.c
@@ -33,7 +33,6 @@
 #include <util.h>
 #include <version.h>
 
-#include <workflow/reboot.h>
 #include <workflow/restore.h>
 #include <workflow/workflow.h>
 
@@ -105,14 +104,6 @@ static commander_error_t _api_restore_backup(const RestoreBackupRequest* request
 }
 #endif
 
-static commander_error_t _api_reboot(void)
-{
-    if (!workflow_reboot()) {
-        return COMMANDER_ERR_GENERIC;
-    }
-    return COMMANDER_OK;
-}
-
 // ------------------------------------ Process ------------------------------------- //
 
 /**
@@ -156,9 +147,6 @@ static commander_error_t _api_process(const Request* request, Response* response
         response->which_response = Response_success_tag;
         return commander_bitboxbase(&(request->request.bitboxbase));
 #endif
-    case Request_reboot_tag:
-        response->which_response = Response_success_tag;
-        return _api_reboot();
     default:
         screen_print_debug("command unknown", 1000);
         return COMMANDER_ERR_INVALID_INPUT;
diff --git a/src/rust/bitbox02-rust/src/hww/api.rs b/src/rust/bitbox02-rust/src/hww/api.rs
@@ -33,6 +33,7 @@ mod set_device_name;
 mod set_mnemonic_passphrase_enabled;
 mod set_password;
 mod show_mnemonic;
+mod system;
 
 use alloc::vec::Vec;
 
@@ -120,6 +121,7 @@ async fn process_api_btc(_request: &Request) -> Option<Result<Response, Error>>
 /// the C commander.
 async fn process_api(request: &Request) -> Option<Result<Response, Error>> {
     match request {
+        Request::Reboot(ref request) => Some(system::reboot(request).await),
         Request::DeviceInfo(_) => Some(device_info::process()),
         Request::DeviceName(ref request) => Some(set_device_name::process(request).await),
         Request::SetPassword(ref request) => Some(set_password::process(request).await),
diff --git a/src/rust/bitbox02-rust/src/hww/api/system.rs b/src/rust/bitbox02-rust/src/hww/api/system.rs
@@ -0,0 +1,83 @@
+// Copyright 2021 Shift Crypto AG
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use super::Error;
+use crate::pb;
+
+use pb::reboot_request::Purpose;
+use pb::response::Response;
+
+use crate::workflow::confirm;
+
+pub async fn reboot(&pb::RebootRequest { purpose }: &pb::RebootRequest) -> Result<Response, Error> {
+    confirm::confirm(&confirm::Params {
+        title: "",
+        body: match Purpose::from_i32(purpose) {
+            Some(Purpose::Upgrade) => "Proceed to upgrade?",
+            Some(Purpose::Settings) => "Go to\nstartup settings?",
+            // No error, if new client library sends a purpose that we don't understand,
+            // we reboot anyway.
+            None => "Reboot?",
+        },
+        ..Default::default()
+    })
+    .await?;
+    bitbox02::reboot()
+}
+
+#[cfg(test)]
+mod tests {
+    extern crate std;
+    use super::*;
+
+    use crate::bb02_async::block_on;
+    use bitbox02::testing::{mock, Data, MUTEX};
+    use std::boxed::Box;
+
+    #[test]
+    pub fn test_reboot() {
+        let _guard = MUTEX.lock().unwrap();
+
+        mock(Data {
+            ui_confirm_create: Some(Box::new(|_| true)),
+            ..Default::default()
+        });
+        let reboot_called = std::panic::catch_unwind(|| {
+            block_on(reboot(&pb::RebootRequest {
+                purpose: Purpose::Upgrade as _,
+            }))
+            .unwrap();
+        });
+        match reboot_called {
+            Ok(()) => panic!("reboot was not called"),
+            Err(msg) => assert_eq!(msg.downcast_ref::<&str>(), Some(&"reboot called")),
+        }
+    }
+
+    #[test]
+    pub fn test_reboot_aborted() {
+        let _guard = MUTEX.lock().unwrap();
+
+        mock(Data {
+            ui_confirm_create: Some(Box::new(|_| false)),
+            ..Default::default()
+        });
+        assert_eq!(
+            block_on(reboot(&pb::RebootRequest {
+                purpose: Purpose::Upgrade as _
+            })),
+            Err(Error::UserAbort),
+        );
+    }
+}
diff --git a/src/rust/bitbox02-rust/src/shiftcrypto.bitbox02.rs b/src/rust/bitbox02-rust/src/shiftcrypto.bitbox02.rs
@@ -744,6 +744,17 @@ pub struct RandomNumberRequest {
 }
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct RebootRequest {
+    #[prost(enumeration="reboot_request::Purpose", tag="1")]
+    pub purpose: i32,
+}
+/// Nested message and enum types in `RebootRequest`.
+pub mod reboot_request {
+    #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
+    #[repr(i32)]
+    pub enum Purpose {
+        Upgrade = 0,
+        Settings = 1,
+    }
 }
 /// Deprecated, last used in v1.0.0
 #[derive(Clone, PartialEq, ::prost::Message)]
diff --git a/src/rust/bitbox02-sys/wrapper.h b/src/rust/bitbox02-sys/wrapper.h
diff --git a/src/rust/bitbox02/src/lib.rs b/src/rust/bitbox02/src/lib.rs
diff --git a/src/system.c b/src/system.c
diff --git a/src/system.h b/src/system.h

Original file line number	Diff line number	Diff line change
`@@ -54,5 +54,6 @@`
`54`	`54`	`btc,`
`55`	`55`	`common,`
`56`	`56`	`eth,`
	`57`	`+ system,`
`57`	`58`	`)`
`58`	`59`	`from .bootloader import Bootloader`