Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Read/Write traits with no_std #120

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/embedded.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Test Embedded

on:
push:

jobs:
build:
runs-on: ubuntu-latest
steps:
-
name: Checkout
uses: actions/checkout@v2
- name: Checkout Toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
components: rust-src
target: thumbv7m-none-eabi
-
name: Build
run: cd embedded && cargo build
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ name = "bitcoin_hashes"
path = "src/lib.rs"

[features]
default = [ "std" ]
std = []
no_std = ["core2"]
serde-std = ["serde/std"]
unstable = [] # for benchmarking

[dependencies]
serde = { version = "1.0", default-features = false, optional = true }
core2 = { version = "0.3.0-alpha.1", default-features = false, optional = true }

[dev-dependencies]
serde_test = "1.0"
Expand Down
26 changes: 26 additions & 0 deletions embedded/.cargo/config
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
[target.thumbv7m-none-eabi]
# uncomment this to make `cargo run` execute programs on QEMU
runner = "qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel"

rustflags = [
# This is needed if your flash or ram addresses are not aligned to 0x10000 in memory.x
# See https://github.com/rust-embedded/cortex-m-quickstart/pull/95
"-C", "link-arg=--nmagic",

# LLD (shipped with the Rust toolchain) is used as the default linker
"-C", "link-arg=-Tlink.x",

# if you run into problems with LLD switch to the GNU linker by commenting out
# this line
# "-C", "linker=arm-none-eabi-ld",

# if you need to link to pre-compiled C libraries provided by a C toolchain
# use GCC as the linker by commenting out both lines above and then
# uncommenting the three lines below
# "-C", "linker=arm-none-eabi-gcc",
# "-C", "link-arg=-Wl,-Tlink.x",
# "-C", "link-arg=-nostartfiles",
]

[build]
target = "thumbv7m-none-eabi" # Cortex-M3
38 changes: 38 additions & 0 deletions embedded/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
[package]
authors = ["Riccardo Casatta <riccardo@casatta.it>"]
edition = "2018"
readme = "README.md"
name = "embedded"
version = "0.1.0"

[dependencies]
cortex-m = "0.6.0"
cortex-m-rt = "0.6.10"
cortex-m-semihosting = "0.3.3"
panic-halt = "0.2.0"
bitcoin_hashes = { path="../", features = ["no_std"] }
alloc-cortex-m = "0.4.1"

# Uncomment for the panic example.
# panic-itm = "0.4.1"

# Uncomment for the allocator example.
# alloc-cortex-m = "0.4.0"

# Uncomment for the device example.
# Update `memory.x`, set target to `thumbv7em-none-eabihf` in `.cargo/config`,
# and then use `cargo build --examples device` to build it.
# [dependencies.stm32f3]
# features = ["stm32f303", "rt"]
# version = "0.7.1"

# this lets you use `cargo fix`!
[[bin]]
name = "embedded"
test = false
bench = false

[profile.release]
codegen-units = 1 # better optimizations
debug = true # symbols are nice and they don't increase the size on Flash
lto = true # better optimizations
31 changes: 31 additions & 0 deletions embedded/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//! This build script copies the `memory.x` file from the crate root into
//! a directory where the linker can always find it at build time.
//! For many projects this is optional, as the linker always searches the
//! project root directory -- wherever `Cargo.toml` is. However, if you
//! are using a workspace or have a more complicated build setup, this
//! build script becomes required. Additionally, by requesting that
//! Cargo re-run the build script whenever `memory.x` is changed,
//! updating `memory.x` ensures a rebuild of the application with the
//! new memory settings.

use std::env;
use std::fs::File;
use std::io::Write;
use std::path::PathBuf;

fn main() {
// Put `memory.x` in our output directory and ensure it's
// on the linker search path.
let out = &PathBuf::from(env::var_os("OUT_DIR").unwrap());
File::create(out.join("memory.x"))
.unwrap()
.write_all(include_bytes!("memory.x"))
.unwrap();
println!("cargo:rustc-link-search={}", out.display());

// By default, Cargo will re-run a build script whenever
// any file in the project changes. By specifying `memory.x`
// here, we ensure the build script is only re-run when
// `memory.x` is changed.
println!("cargo:rerun-if-changed=memory.x");
}
34 changes: 34 additions & 0 deletions embedded/memory.x
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
MEMORY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not an easier way to test embedded stuff than four config files just for a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, like #119 (comment)

however, this way is suggested in the rust embedded book from the rust embedded team, so maybe more common for embedded devs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .cargo/config could be easy removed in favor of command parameters and .github/workflows/embedded.yml could be mixed with the other github actions. Would that be ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we need to squash the workflows files down, but if its possible to remove some of the config files it would be nice, especially given a few of them look like half-comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#119 is updated without comments and fewer config files. Could you please review it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superseded by #122

{
/* NOTE 1 K = 1 KiBi = 1024 bytes */
/* TODO Adjust these memory regions to match your device memory layout */
/* These values correspond to the LM3S6965, one of the few devices QEMU can emulate */
FLASH : ORIGIN = 0x00000000, LENGTH = 256K
RAM : ORIGIN = 0x20000000, LENGTH = 64K
}

/* This is where the call stack will be allocated. */
/* The stack is of the full descending type. */
/* You may want to use this variable to locate the call stack and static
variables in different memory regions. Below is shown the default value */
/* _stack_start = ORIGIN(RAM) + LENGTH(RAM); */

/* You can use this symbol to customize the location of the .text section */
/* If omitted the .text section will be placed right after the .vector_table
section */
/* This is required only on microcontrollers that store some configuration right
after the vector table */
/* _stext = ORIGIN(FLASH) + 0x400; */

/* Example of putting non-initialized variables into custom RAM locations. */
/* This assumes you have defined a region RAM2 above, and in the Rust
sources added the attribute `#[link_section = ".ram2bss"]` to the data
you want to place there. */
/* Note that the section will not be zero-initialized by the runtime! */
/* SECTIONS {
.ram2bss (NOLOAD) : ALIGN(4) {
*(.ram2bss);
. = ALIGN(4);
} > RAM2
} INSERT AFTER .bss;
*/
56 changes: 56 additions & 0 deletions embedded/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#![feature(alloc_error_handler)]
#![no_main]
#![no_std]

#[macro_use]
extern crate bitcoin_hashes;

extern crate alloc;
use panic_halt as _;

use self::alloc::string::ToString;
use self::alloc::vec;
use self::alloc::vec::Vec;
use core::alloc::Layout;

use alloc_cortex_m::CortexMHeap;
use cortex_m::asm;
use cortex_m_rt::entry;
use cortex_m_semihosting::{debug, hprintln};

use bitcoin_hashes::core2::io::Write;
use bitcoin_hashes::sha256;
use bitcoin_hashes::Hash;

hash_newtype!(TestType, sha256::Hash, 32, doc = "test");

// this is the allocator the application will use
#[global_allocator]
static ALLOCATOR: CortexMHeap = CortexMHeap::empty();

const HEAP_SIZE: usize = 1024; // in bytes

#[entry]
fn main() -> ! {
// Initialize the allocator BEFORE you use it
unsafe { ALLOCATOR.init(cortex_m_rt::heap_start() as usize, HEAP_SIZE) }

let mut engine = sha256::Hash::engine();
engine.write_all(&[]).unwrap();
let hash = sha256::Hash::from_engine(engine);
hprintln!("hash {}", a).unwrap();

// exit QEMU
// NOTE do not run this on hardware; it can corrupt OpenOCD state
debug::exit(debug::EXIT_SUCCESS);

loop {}
}

// define what happens in an Out Of Memory (OOM) condition
#[alloc_error_handler]
fn alloc_error(_layout: Layout) -> ! {
asm::bkpt();

loop {}
}
7 changes: 3 additions & 4 deletions src/hex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
use core::{fmt, str};
use Hash;

#[cfg(all(feature = "no_std", not(test)))]
use alloc::{string::String, format, vec::Vec};

/// Hex decoding error
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum Error {
Expand All @@ -40,7 +43,6 @@ impl fmt::Display for Error {
}

/// Trait for objects that can be serialized as hex strings
#[cfg(any(test, feature = "std"))]
pub trait ToHex {
/// Hex representation of the object
fn to_hex(&self) -> String;
Expand All @@ -60,7 +62,6 @@ pub trait FromHex: Sized {
}
}

#[cfg(any(test, feature = "std"))]
impl<T: fmt::LowerHex> ToHex for T {
/// Outputs the hash in hexadecimal form
fn to_hex(&self) -> String {
Expand Down Expand Up @@ -174,7 +175,6 @@ pub fn format_hex_reverse(data: &[u8], f: &mut fmt::Formatter) -> fmt::Result {
Ok(())
}

#[cfg(any(test, feature = "std"))]
impl ToHex for [u8] {
fn to_hex(&self) -> String {
use core::fmt::Write;
Expand All @@ -186,7 +186,6 @@ impl ToHex for [u8] {
}
}

#[cfg(any(test, feature = "std"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to just have a pub function that converts these objects to hex and then only have the FromHex/etc traits on std-mode, that way its usable either way, but we don't take a new dependency just for hex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are referring to the new dependency core2, that is for Read/Write traits used in impls.rs

If you are referring to alloc, yes it may be worthy to avoid using alloc just for this traits

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to core2 (indeed, alloc isn't really a "new" dependency, as its part of std anyway) - an alternative approach to avoid core2 would be to reimplement the io::Write/Read traits as functions, with versions which expose Write/Read if we're building with std. Ultimately a user not using std isn't going to have Write or Read anyway, they just want a byte buffer (or a string, in the case of hex), which we can just give them, instead of exposing the Write or Read traits (which they're unlikely to use anyway - they don't have std so probably no files, just writing into Vecs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#122 is not using core2 and it is simply using input function from the Engine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt I don't think it's realistic to expect people not to use Read or Write, these are universal traits used by all sorts of things, and it looks like core2 is the "standard" way to do this without std. So I think we have to bite the bullet and just use it, even though it bumps our MSRV for nostd and even though it's an extra dep.

We could plausibly get away without it here, or maybe feature-gate it even, but for rust-bitcoin I think we'll need it no matter what, unless we want to either disable all the decoding/encoding stuff (i.e. most of the library) or rewrite it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I see what you're saying. If some 3rd party writes a fn myfunc<W: Write>(thing: W) and we have elsewhere impl<T: OurCustomNoStdWrite> Write for T { ... } then people can use our types when calling myfunc and not care that we did our own weird thing.

@RCasatta I've changed my mind to agree with Matt now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rust-bitcoin would need to replace every std::io::Write with our OurCustomNoStdWrite from bitcoin-hashes, is it correct?

(rust-bech32 doesn't use Write, otherwise, it would be somehow out of context here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is correct. Similar to what we did had with Encodable prior to replacing that with io::Write

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting discussion. I didn't see this before opening the following PR in rust-bitcoin with a similar approach:

https://github.com/rust-bitcoin/rust-bitcoin/pull/603/files#diff-76866598ce8fd16261a27ac58a84b2825e6e77fc37c163a6afa60f0f4477e569R28

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note that I rename it to just Write when I import it into the root - in case you are looking at the rest of the PR)

impl FromHex for Vec<u8> {
fn from_byte_iter<I>(iter: I) -> Result<Self, Error>
where I: Iterator<Item=Result<u8, Error>> +
Expand Down
11 changes: 9 additions & 2 deletions src/std_impls.rs → src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,25 @@
//!
//! impls of traits defined in `std` and not `core`

#[cfg(any(not(feature="no_std"), test))]
use std::{error, io};
#[cfg(all(feature = "no_std", not(test)))]
use core2::io as io;

use {hex, sha1, sha256, sha512, ripemd160, siphash24};
use {sha1, sha256, sha512, ripemd160, siphash24};
use HashEngine;

#[cfg(not(feature="no_std"))]
use Error;

#[cfg(not(feature="no_std"))]
impl error::Error for Error {
fn cause(&self) -> Option<&error::Error> { None }
fn description(&self) -> &str { "`std::error::description` is deprecated" }
}

impl error::Error for hex::Error {
#[cfg(not(feature="no_std"))]
impl error::Error for ::hex::Error {
fn cause(&self) -> Option<&error::Error> { None }
fn description(&self) -> &str { "`std::error::description` is deprecated" }
}
Expand Down
12 changes: 9 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,23 @@
#![allow(bare_trait_objects)]
#![allow(ellipsis_inclusive_range_patterns)]

#![cfg_attr(all(not(test), not(feature = "std")), no_std)]
#![cfg_attr(all(not(test), feature = "no_std"), no_std)]
#![cfg_attr(all(test, feature = "unstable"), feature(test))]
#[cfg(all(test, feature = "unstable"))] extern crate test;

#[cfg(any(test, feature="std"))] pub extern crate core;
#[cfg(any(test, not(feature="no_std")))] extern crate core;
#[cfg(feature="serde")] pub extern crate serde;
#[cfg(all(test,feature="serde"))] extern crate serde_test;

#[cfg(all(feature = "no_std", not(test)))]
extern crate alloc;

#[cfg(all(feature = "no_std", not(test)))]
pub extern crate core2;

#[macro_use] mod util;
#[macro_use] pub mod serde_macros;
#[cfg(any(test, feature = "std"))] mod std_impls;
mod impls;
pub mod error;
pub mod hex;
pub mod hash160;
Expand Down
Loading