Add validation to esp-config (#2475)

* Clean up and re-organize `lib.rs` a bit (sorry)

* Add configuration validation, some other refactoring/improvements

* Add a built-in range validator, re-document the `generate_config` function

* Update build scripts to reflect new configuration API

esp-config/src/generate.rs

@@ -1,77 +1,246 @@
-use core::fmt::Write;
-use std::{collections::HashMap, env, fs, path::PathBuf};
+use std::{
+    collections::HashMap,
+    env,
+    fmt::{self, Write as _},
+    fs,
+    ops::Range,
+    path::PathBuf,
+};
 
 const DOC_TABLE_HEADER: &str = r#"
 | Name | Description | Default value |
 |------|-------------|---------------|
 "#;
-const CHOSEN_TABLE_HEADER: &str = r#"
+
+const SELECTED_TABLE_HEADER: &str = r#"
 | Name | Selected value |
 |------|----------------|
 "#;
 
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub struct ParseError(String);
-
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub enum Value {
-    SignedInteger(isize),
-    UnsignedInteger(usize),
-    Bool(bool),
-    String(String),
+/// Configuration errors.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum Error {
+    /// Parse errors.
+    Parse(String),
+    /// Validation errors.
+    Validation(String),
 }
 
-impl Value {
-    fn parse_in_place(&mut self, s: &str) -> Result<(), ParseError> {
-        *self = match self {
-            Value::Bool(_) => match s.to_lowercase().as_ref() {
-                "false" | "no" | "n" | "f" => Value::Bool(false),
-                "true" | "yes" | "y" | "t" => Value::Bool(true),
-                _ => return Err(ParseError(format!("Invalid boolean value: {}", s))),
-            },
-            Value::SignedInteger(_) => Value::SignedInteger(
-                match s.as_bytes() {
-                    [b'0', b'x', ..] => isize::from_str_radix(&s[2..], 16),
-                    [b'0', b'o', ..] => isize::from_str_radix(&s[2..], 8),
-                    [b'0', b'b', ..] => isize::from_str_radix(&s[2..], 2),
-                    _ => isize::from_str_radix(&s, 10),
-                }
-                .map_err(|_| ParseError(format!("Invalid signed numerical value: {}", s)))?,
-            ),
-            Value::UnsignedInteger(_) => Value::UnsignedInteger(
-                match s.as_bytes() {
-                    [b'0', b'x', ..] => usize::from_str_radix(&s[2..], 16),
-                    [b'0', b'o', ..] => usize::from_str_radix(&s[2..], 8),
-                    [b'0', b'b', ..] => usize::from_str_radix(&s[2..], 2),
-                    _ => usize::from_str_radix(&s, 10),
-                }
-                .map_err(|_| ParseError(format!("Invalid unsigned numerical value: {}", s)))?,
-            ),
-            Value::String(_) => Value::String(String::from(s)),
-        };
-        Ok(())
+impl Error {
+    /// Convenience function for creating parse errors.
+    pub fn parse<S>(message: S) -> Self
+    where
+        S: Into<String>,
+    {
+        Self::Parse(message.into())
     }
 
-    fn as_string(&self) -> String {
+    /// Convenience function for creating validation errors.
+    pub fn validation<S>(message: S) -> Self
+    where
+        S: Into<String>,
+    {
+        Self::Validation(message.into())
+    }
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Value::Bool(value) => String::from(if *value { "true" } else { "false" }),
-            Value::SignedInteger(value) => format!("{}", value),
-            Value::UnsignedInteger(value) => format!("{}", value),
-            Value::String(value) => value.clone(),
+            Error::Parse(message) => write!(f, "{message}"),
+            Error::Validation(message) => write!(f, "{message}"),
         }
     }
 }
 
-/// Generate and parse config from a prefix, and array of key, default,
-/// description tuples.
+/// Supported configuration value types.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum Value {
+    /// Booleans.
+    Bool(bool),
+    /// Integers.
+    Integer(i128),
+    /// Strings.
+    String(String),
+}
+
+// TODO: Do we want to handle negative values for non-decimal values?
+impl Value {
+    fn parse_in_place(&mut self, s: &str) -> Result<(), Error> {
+        *self = match self {
+            Value::Bool(_) => match s {
+                "true" => Value::Bool(true),
+                "false" => Value::Bool(false),
+                _ => {
+                    return Err(Error::parse(format!(
+                        "Expected 'true' or 'false', found: '{s}'"
+                    )))
+                }
+            },
+            Value::Integer(_) => {
+                let inner = match s.as_bytes() {
+                    [b'0', b'x', ..] => i128::from_str_radix(&s[2..], 16),
+                    [b'0', b'o', ..] => i128::from_str_radix(&s[2..], 8),
+                    [b'0', b'b', ..] => i128::from_str_radix(&s[2..], 2),
+                    _ => i128::from_str_radix(&s, 10),
+                }
+                .map_err(|_| Error::parse(format!("Expected valid intger value, found: '{s}'")))?;
+
+                Value::Integer(inner)
+            }
+            Value::String(_) => Value::String(s.into()),
+        };
+
+        Ok(())
+    }
+
+    /// Convert the value to a [bool].
+    pub fn as_bool(&self) -> bool {
+        match self {
+            Value::Bool(value) => *value,
+            _ => panic!("attempted to convert non-bool value to a bool"),
+        }
+    }
+
+    /// Convert the value to an [i128].
+    pub fn as_integer(&self) -> i128 {
+        match self {
+            Value::Integer(value) => *value,
+            _ => panic!("attempted to convert non-integer value to an integer"),
+        }
+    }
+
+    /// Convert the value to a [String].
+    pub fn as_string(&self) -> String {
+        match self {
+            Value::String(value) => value.to_owned(),
+            _ => panic!("attempted to convert non-string value to a string"),
+        }
+    }
+
+    /// Is the value a bool?
+    pub fn is_bool(&self) -> bool {
+        matches!(self, Value::Bool(_))
+    }
+
+    /// Is the value an integer?
+    pub fn is_integer(&self) -> bool {
+        matches!(self, Value::Integer(_))
+    }
+
+    /// Is the value a string?
+    pub fn is_string(&self) -> bool {
+        matches!(self, Value::String(_))
+    }
+}
+
+impl fmt::Display for Value {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Value::Bool(b) => write!(f, "{b}"),
+            Value::Integer(i) => write!(f, "{i}"),
+            Value::String(s) => write!(f, "{s}"),
+        }
+    }
+}
+
+/// Configuration value validation functions.
+pub enum Validator {
+    /// Only allow negative integers, i.e. any values less than 0.
+    NegativeInteger,
+    /// Only allow non-negative integers, i.e. any values greater than or equal
+    /// to 0.
+    NonNegativeInteger,
+    /// Only allow positive integers, i.e. any values greater than to 0.
+    PositiveInteger,
+    /// Ensure that an integer value falls within the specified range.
+    IntegerInRange(Range<i128>),
+    /// A custom validation function to run against any supported value type.
+    Custom(Box<dyn Fn(&Value) -> Result<(), Error>>),
+}
+
+impl Validator {
+    fn validate(&self, value: &Value) -> Result<(), Error> {
+        match self {
+            Validator::NegativeInteger => negative_integer(value)?,
+            Validator::NonNegativeInteger => non_negative_integer(value)?,
+            Validator::PositiveInteger => positive_integer(value)?,
+            Validator::IntegerInRange(range) => integer_in_range(range, value)?,
+            Validator::Custom(validator_fn) => validator_fn(value)?,
+        }
+
+        Ok(())
+    }
+}
+
+fn negative_integer(value: &Value) -> Result<(), Error> {
+    if !value.is_integer() {
+        return Err(Error::validation(
+            "Validator::NegativeInteger can only be used with integer values",
+        ));
+    } else if value.as_integer() >= 0 {
+        return Err(Error::validation(format!(
+            "Expected negative integer, found '{}'",
+            value.as_integer()
+        )));
+    }
+
+    Ok(())
+}
+
+fn non_negative_integer(value: &Value) -> Result<(), Error> {
+    if !value.is_integer() {
+        return Err(Error::validation(
+            "Validator::NonNegativeInteger can only be used with integer values",
+        ));
+    } else if value.as_integer() < 0 {
+        return Err(Error::validation(format!(
+            "Expected non-negative integer, found '{}'",
+            value.as_integer()
+        )));
+    }
+
+    Ok(())
+}
+
+fn positive_integer(value: &Value) -> Result<(), Error> {
+    if !value.is_integer() {
+        return Err(Error::validation(
+            "Validator::PositiveInteger can only be used with integer values",
+        ));
+    } else if value.as_integer() <= 0 {
+        return Err(Error::validation(format!(
+            "Expected positive integer, found '{}'",
+            value.as_integer()
+        )));
+    }
+
+    Ok(())
+}
+
+fn integer_in_range(range: &Range<i128>, value: &Value) -> Result<(), Error> {
+    if !value.is_integer() || !range.contains(&value.as_integer()) {
+        Err(Error::validation(format!(
+            "Value '{}' does not fall within range '{:?}'",
+            value, range
+        )))
+    } else {
+        Ok(())
+    }
+}
+
+/// Generate and parse config from a prefix, and an array tuples containing the
+/// name, description, default value, and an optional validator.
 ///
 /// This function will parse any `SCREAMING_SNAKE_CASE` environment variables
-/// that match the given prefix. It will then attempt to parse the [`Value`].
+/// that match the given prefix. It will then attempt to parse the [`Value`] and
+/// run any validators which have been specified.
+///
 /// Once the config has been parsed, this function will emit `snake_case` cfg's
 /// _without_ the prefix which can be used in the dependant crate. After that,
 /// it will create a markdown table in the `OUT_DIR` under the name
-/// `{prefix}_config_table.md` where prefix has also been converted
-/// to `snake_case`. This can be included in crate documentation to outline the
+/// `{prefix}_config_table.md` where prefix has also been converted to
+/// `snake_case`. This can be included in crate documentation to outline the
 /// available configuration options for the crate.
 ///
 /// Passing a value of true for the `emit_md_tables` argument will create and
@@ -81,22 +250,46 @@ impl Value {
 /// Unknown keys with the supplied prefix will cause this function to panic.
 pub fn generate_config(
     prefix: &str,
-    config: &[(&str, Value, &str)],
+    config: &[(&str, &str, Value, Option<Validator>)],
     emit_md_tables: bool,
 ) -> HashMap<String, Value> {
-    // only rebuild if build.rs changed. Otherwise Cargo will rebuild if any
+    // Only rebuild if `build.rs` changed. Otherwise, Cargo will rebuild if any
     // other file changed.
     println!("cargo:rerun-if-changed=build.rs");
+
     #[cfg(not(test))]
     env_change_work_around();
 
-    // ensure that the prefix is `SCREAMING_SNAKE_CASE`
-    let prefix = format!("{}_", screaming_snake_case(prefix));
     let mut doc_table = String::from(DOC_TABLE_HEADER);
-    let mut selected_config = String::from(CHOSEN_TABLE_HEADER);
+    let mut selected_config = String::from(SELECTED_TABLE_HEADER);
+
+    // Ensure that the prefix is `SCREAMING_SNAKE_CASE`:
+    let prefix = screaming_snake_case(prefix);
+
+    // Build a lookup table for any provided validators; we must prefix the
+    // name of the config and transform it to SCREAMING_SNAKE_CASE so that
+    // it matches the keys in the hash table produced by `create_config`.
+    let config_validators = config
+        .iter()
+        .flat_map(|(name, _description, _default, validator)| {
+            if let Some(validator) = validator {
+                let name = format!("{prefix}_{}", screaming_snake_case(name));
+                Some((name, validator))
+            } else {
+                None
+            }
+        })
+        .collect::<HashMap<_, _>>();
 
     let mut configs = create_config(&prefix, config, &mut doc_table);
     capture_from_env(&prefix, &mut configs);
+
+    for (name, value) in configs.iter() {
+        if let Some(validator) = config_validators.get(name) {
+            validator.validate(value).unwrap();
+        }
+    }
+
     emit_configuration(&prefix, &configs, &mut selected_config);
 
     if emit_md_tables {
@@ -118,8 +311,7 @@ fn env_change_work_around() {
     // target
     while !out_dir.ends_with("target") {
         if !out_dir.pop() {
-            // We ran out of directories...
-            return;
+            return; // We ran out of directories...
         }
     }
     out_dir.pop();
@@ -141,44 +333,42 @@ fn env_change_work_around() {
     }
 }
 
-fn emit_configuration(
+fn create_config(
     prefix: &str,
-    configs: &HashMap<String, Value>,
-    selected_config: &mut String,
-) {
-    // emit cfgs and set envs
-    for (name, value) in configs.into_iter() {
-        let cfg_name = snake_case(name.trim_start_matches(prefix));
-        println!("cargo:rustc-check-cfg=cfg({cfg_name})");
-        match value {
-            Value::Bool(true) => {
-                println!("cargo:rustc-cfg={cfg_name}")
-            }
-            _ => {}
-        }
+    config: &[(&str, &str, Value, Option<Validator>)],
+    doc_table: &mut String,
+) -> HashMap<String, Value> {
+    let mut configs = HashMap::new();
 
-        let value = value.as_string();
-        // values that haven't been seen will be output here with the default value
-        println!("cargo:rustc-env={}={}", name, value);
+    for (name, description, default, _validator) in config {
+        let name = format!("{prefix}_{}", screaming_snake_case(name));
+        configs.insert(name.clone(), default.clone());
 
-        writeln!(selected_config, "|**{name}**|{value}|").unwrap();
+        // Write documentation table line:
+        let default = default.to_string();
+        writeln!(doc_table, "|**{name}**|{description}|{default}|").unwrap();
+
+        // Rebuild if config environment variable changed:
+        println!("cargo:rerun-if-env-changed={name}");
     }
+
+    configs
 }
 
 fn capture_from_env(prefix: &str, configs: &mut HashMap<String, Value>) {
     let mut unknown = Vec::new();
     let mut failed = Vec::new();
 
-    // Try and capture input from the environment
+    // Try and capture input from the environment:
     for (var, value) in env::vars() {
-        if let Some(_) = var.strip_prefix(prefix) {
+        if var.strip_prefix(prefix).is_some() {
             let Some(cfg) = configs.get_mut(&var) else {
                 unknown.push(var);
                 continue;
             };
 
             if let Err(e) = cfg.parse_in_place(&value) {
-                failed.push(format!("{}: {e:?}", var));
+                failed.push(format!("{var}: {e}"));
             }
         }
     }
@@ -192,61 +382,54 @@ fn capture_from_env(prefix: &str, configs: &mut HashMap<String, Value>) {
     }
 }
 
-fn create_config(
+fn emit_configuration(
     prefix: &str,
-    config: &[(&str, Value, &str)],
-    doc_table: &mut String,
-) -> HashMap<String, Value> {
-    // Generate the template for the config
-    let mut configs = HashMap::new();
-    for (name, default, desc) in config {
-        let name = format!("{prefix}{}", screaming_snake_case(&name));
-        configs.insert(name.clone(), default.clone());
+    configs: &HashMap<String, Value>,
+    selected_config: &mut String,
+) {
+    for (name, value) in configs.iter() {
+        let cfg_name = snake_case(name.trim_start_matches(&format!("{prefix}_")));
+        println!("cargo:rustc-check-cfg=cfg({cfg_name})");
 
-        // write doc table line
-        let default = default.as_string();
-        writeln!(doc_table, "|**{name}**|{desc}|{default}|").unwrap();
+        if let Value::Bool(true) = value {
+            println!("cargo:rustc-cfg={cfg_name}");
+        }
 
-        // Rebuild if config envvar changed.
-        println!("cargo:rerun-if-env-changed={name}");
+        let value = value.to_string();
+
+        // Values that haven't been seen will be output here with the default value:
+        println!("cargo:rustc-env={}={}", name, value);
+        writeln!(selected_config, "|**{name}**|{value}|").unwrap();
     }
-
-    configs
 }
 
 fn write_config_tables(prefix: &str, doc_table: String, selected_config: String) {
     let out_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap());
+
     let out_file = out_dir
-        .join(format!("{prefix}config_table.md"))
-        .to_string_lossy()
+        .join(format!("{prefix}_config_table.md"))
+        .display()
         .to_string();
     fs::write(out_file, doc_table).unwrap();
 
-    let out_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap());
     let out_file = out_dir
-        .join(format!("{prefix}selected_config.md"))
-        .to_string_lossy()
+        .join(format!("{prefix}_selected_config.md"))
+        .display()
         .to_string();
     fs::write(out_file, selected_config).unwrap();
 }
 
-// Converts a symbol name like
-// "PLACE-spi_DRIVER-IN_ram"
-// to
-// "place_spi_driver_in_ram"
 fn snake_case(name: &str) -> String {
     let mut name = name.replace("-", "_");
     name.make_ascii_lowercase();
+
     name
 }
 
-// Converts a symbol name like
-// "PLACE-spi_DRIVER-IN_ram"
-// to
-// "PLACE_SPI_DRIVER_IN_RAM"
 fn screaming_snake_case(name: &str) -> String {
     let mut name = name.replace("-", "_");
     name.make_ascii_uppercase();
+
     name
 }
 
@@ -257,32 +440,24 @@ mod test {
     #[test]
     fn value_number_formats() {
         const INPUTS: &[&str] = &["0xAA", "0o252", "0b0000000010101010", "170"];
-        let mut v = Value::SignedInteger(0);
+        let mut v = Value::Integer(0);
 
         for input in INPUTS {
             v.parse_in_place(input).unwrap();
             // no matter the input format, the output format should be decimal
-            assert_eq!(v.as_string(), "170");
+            assert_eq!(format!("{v}"), "170");
         }
     }
 
     #[test]
     fn value_bool_inputs() {
-        const TRUE_INPUTS: &[&str] = &["true", "y", "yes"];
-        const FALSE_INPUTS: &[&str] = &["false", "n", "no"];
         let mut v = Value::Bool(false);
 
-        for input in TRUE_INPUTS {
-            v.parse_in_place(input).unwrap();
-            // no matter the input variant, the output format should be "true"
-            assert_eq!(v.as_string(), "true");
-        }
+        v.parse_in_place("true").unwrap();
+        assert_eq!(format!("{v}"), "true");
 
-        for input in FALSE_INPUTS {
-            v.parse_in_place(input).unwrap();
-            // no matter the input variant, the output format should be "false"
-            assert_eq!(v.as_string(), "false");
-        }
+        v.parse_in_place("false").unwrap();
+        assert_eq!(format!("{v}"), "false");
     }
 
     #[test]
@@ -298,13 +473,18 @@ mod test {
                 let configs = generate_config(
                     "esp-test",
                     &[
-                        ("number", Value::UnsignedInteger(999), "NA"),
-                        ("number_signed", Value::SignedInteger(-777), "NA"),
-                        ("string", Value::String("Demo".to_owned()), "NA"),
-                        ("bool", Value::Bool(false), "NA"),
-                        ("number_default", Value::UnsignedInteger(999), "NA"),
-                        ("string_default", Value::String("Demo".to_owned()), "NA"),
-                        ("bool_default", Value::Bool(false), "NA"),
+                        ("number", "NA", Value::Integer(999), None),
+                        ("number_signed", "NA", Value::Integer(-777), None),
+                        ("string", "NA", Value::String("Demo".to_owned()), None),
+                        ("bool", "NA", Value::Bool(false), None),
+                        ("number_default", "NA", Value::Integer(999), None),
+                        (
+                            "string_default",
+                            "NA",
+                            Value::String("Demo".to_owned()),
+                            None,
+                        ),
+                        ("bool_default", "NA", Value::Bool(false), None),
                     ],
                     false,
                 );
@@ -312,14 +492,14 @@ mod test {
                 // some values have changed
                 assert_eq!(
                     match configs.get("ESP_TEST_NUMBER").unwrap() {
-                        Value::UnsignedInteger(num) => *num,
+                        Value::Integer(num) => *num,
                         _ => unreachable!(),
                     },
                     0xaa
                 );
                 assert_eq!(
                     match configs.get("ESP_TEST_NUMBER_SIGNED").unwrap() {
-                        Value::SignedInteger(num) => *num,
+                        Value::Integer(num) => *num,
                         _ => unreachable!(),
                     },
                     -999
@@ -342,7 +522,7 @@ mod test {
                 // the rest are the defaults
                 assert_eq!(
                     match configs.get("ESP_TEST_NUMBER_DEFAULT").unwrap() {
-                        Value::UnsignedInteger(num) => *num,
+                        Value::Integer(num) => *num,
                         _ => unreachable!(),
                     },
                     999
@@ -365,6 +545,114 @@ mod test {
         )
     }
 
+    #[test]
+    fn builtin_validation_passes() {
+        temp_env::with_vars(
+            [
+                ("ESP_TEST_POSITIVE_NUMBER", Some("7")),
+                ("ESP_TEST_NEGATIVE_NUMBER", Some("-1")),
+                ("ESP_TEST_NON_NEGATIVE_NUMBER", Some("0")),
+                ("ESP_TEST_RANGE", Some("9")),
+            ],
+            || {
+                generate_config(
+                    "esp-test",
+                    &[
+                        (
+                            "positive_number",
+                            "NA",
+                            Value::Integer(-1),
+                            Some(Validator::PositiveInteger),
+                        ),
+                        (
+                            "negative_number",
+                            "NA",
+                            Value::Integer(1),
+                            Some(Validator::NegativeInteger),
+                        ),
+                        (
+                            "non_negative_number",
+                            "NA",
+                            Value::Integer(-1),
+                            Some(Validator::NonNegativeInteger),
+                        ),
+                        (
+                            "range",
+                            "NA",
+                            Value::Integer(0),
+                            Some(Validator::IntegerInRange(5..10)),
+                        ),
+                    ],
+                    false,
+                )
+            },
+        );
+    }
+
+    #[test]
+    fn custom_validation_passes() {
+        temp_env::with_vars([("ESP_TEST_NUMBER", Some("13"))], || {
+            generate_config(
+                "esp-test",
+                &[(
+                    "number",
+                    "NA",
+                    Value::Integer(-1),
+                    Some(Validator::Custom(Box::new(|value| {
+                        let range = 10..20;
+                        if !value.is_integer() || !range.contains(&value.as_integer()) {
+                            Err(Error::validation("value does not fall within range"))
+                        } else {
+                            Ok(())
+                        }
+                    }))),
+                )],
+                false,
+            )
+        });
+    }
+
+    #[test]
+    #[should_panic]
+    fn builtin_validation_bails() {
+        temp_env::with_vars([("ESP_TEST_POSITIVE_NUMBER", Some("-99"))], || {
+            generate_config(
+                "esp-test",
+                &[(
+                    "positive_number",
+                    "NA",
+                    Value::Integer(-1),
+                    Some(Validator::PositiveInteger),
+                )],
+                false,
+            )
+        });
+    }
+
+    #[test]
+    #[should_panic]
+    fn custom_validation_bails() {
+        temp_env::with_vars([("ESP_TEST_NUMBER", Some("37"))], || {
+            generate_config(
+                "esp-test",
+                &[(
+                    "number",
+                    "NA",
+                    Value::Integer(-1),
+                    Some(Validator::Custom(Box::new(|value| {
+                        let range = 10..20;
+                        if !value.is_integer() || !range.contains(&value.as_integer()) {
+                            Err(Error::validation("value does not fall within range"))
+                        } else {
+                            Ok(())
+                        }
+                    }))),
+                )],
+                false,
+            )
+        });
+    }
+
     #[test]
     #[should_panic]
     fn env_unknown_bails() {
@@ -376,7 +664,7 @@ mod test {
             || {
                 generate_config(
                     "esp-test",
-                    &[("number", Value::UnsignedInteger(999), "NA")],
+                    &[("number", "NA", Value::Integer(999), None)],
                     false,
                 );
             },
@@ -389,7 +677,7 @@ mod test {
         temp_env::with_vars([("ESP_TEST_NUMBER", Some("Hello world"))], || {
             generate_config(
                 "esp-test",
-                &[("number", Value::UnsignedInteger(999), "NA")],
+                &[("number", "NA", Value::Integer(999), None)],
                 false,
             );
         });

esp-config/src/lib.rs

@@ -3,17 +3,30 @@
 #![doc = document_features::document_features!(feature_label = r#"<span class="stab portability"><code>{feature}</code></span>"#)]
 #![doc(html_logo_url = "https://avatars.githubusercontent.com/u/46717278")]
 #![cfg_attr(not(feature = "build"), no_std)]
+#![deny(missing_docs, rust_2018_idioms)]
 
 #[cfg(feature = "build")]
 mod generate;
 #[cfg(feature = "build")]
-pub use generate::*;
+pub use generate::{generate_config, Error, Validator, Value};
 
+/// Parse the value of an environment variable as a [bool] at compile time.
+#[macro_export]
+macro_rules! esp_config_bool {
+    ( $var:expr ) => {
+        match env!($var).as_bytes() {
+            b"true" => true,
+            b"false" => false,
+            _ => ::core::panic!("boolean value must be either 'true' or 'false'"),
+        }
+    };
+}
+
+// TODO: From 1.82 on, we can use `<$ty>::from_str_radix(env!($var), 10)`
+/// Parse the value of an environment variable as an integer at compile time.
 #[macro_export]
-// TODO from 1.82 we can use <$ty>::from_str_radix(env!($var), 10) instead
-/// Parse the value of a `env` variable as an integer at compile time
 macro_rules! esp_config_int {
-    ($ty:ty, $var:expr) => {
+    ( $ty:ty, $var:expr ) => {
         const {
             const BYTES: &[u8] = env!($var).as_bytes();
             esp_config_int_parse!($ty, BYTES)
@@ -21,63 +34,53 @@ macro_rules! esp_config_int {
     };
 }
 
+/// Get the string value of an environment variable at compile time.
 #[macro_export]
-/// Get the string value of an `env` variable at compile time
 macro_rules! esp_config_str {
-    ($var:expr) => {
+    ( $var:expr ) => {
         env!($var)
     };
 }
 
-#[macro_export]
-/// Parse the value of a `env` variable as an bool at compile time
-macro_rules! esp_config_bool {
-    ($var:expr) => {
-        match env!($var).as_bytes() {
-            b"false" => false,
-            _ => true,
-        }
-    };
-}
-
-#[macro_export]
-#[doc(hidden)] // to avoid confusion with esp_config_int, let's hide this
 /// Parse a string like "777" into an integer, which _can_ be used in a `const`
 /// context
+#[doc(hidden)] // To avoid confusion with `esp_config_int`, hide this in the docs
+#[macro_export]
 macro_rules! esp_config_int_parse {
-    ($ty:ty, $bytes:expr) => {{
+    ( $ty:ty, $bytes:expr ) => {{
         let mut bytes = $bytes;
         let mut val: $ty = 0;
         let mut sign_seen = false;
         let mut is_negative = false;
+
         while let [byte, rest @ ..] = bytes {
             match *byte {
                 b'0'..=b'9' => {
                     val = val * 10 + (*byte - b'0') as $ty;
                 }
                 b'-' | b'+' if !sign_seen => {
-                    if *byte == b'-' {
-                        is_negative = true;
-                    }
+                    is_negative = *byte == b'-';
                     sign_seen = true;
                 }
-                _ => ::core::panic!("invalid digit"),
+                _ => ::core::panic!("invalid character encountered while parsing integer"),
             }
+
             bytes = rest;
         }
+
         if is_negative {
             let original = val;
-            // subtract twice to get the negative
+            // Subtract the value twice to get a negative:
             val -= original;
             val -= original;
         }
+
         val
     }};
 }
 
 #[cfg(test)]
 mod test {
-
     // We can only test success in the const context
     const _: () = {
         core::assert!(esp_config_int_parse!(i64, "-77777".as_bytes()) == -77777);
@@ -98,4 +101,10 @@ mod test {
     fn test_expect_positive() {
         esp_config_int_parse!(u8, "-5".as_bytes());
     }
+
+    #[test]
+    #[should_panic]
+    fn test_invalid_digit() {
+        esp_config_int_parse!(u32, "a".as_bytes());
+    }
 }

esp-hal-embassy/build.rs

@@ -43,8 +43,9 @@ fn main() -> Result<(), Box<dyn Error>> {
         "esp_hal_embassy",
         &[(
             "low-power-wait",
-            Value::Bool(true),
             "Enables the lower-power wait if no tasks are ready to run on the thread-mode executor. This allows the MCU to use less power if the workload allows. Recommended for battery-powered systems. May impact analog performance.",
+            Value::Bool(true),
+            None
         )],
         true,
     );

esp-hal/build.rs

@@ -80,23 +80,27 @@ fn main() -> Result<(), Box<dyn Error>> {
         &[
             (
                 "place-spi-driver-in-ram",
-                Value::Bool(false),
                 "Places the SPI driver in RAM for better performance",
+                Value::Bool(false),
+                None
             ),
             (
                 "spi-address-workaround",
-                Value::Bool(true),
                 "(ESP32 only) Enables a workaround for the issue where SPI in half-duplex mode incorrectly transmits the address on a single line if the data buffer is empty.",
+                Value::Bool(true),
+                None
             ),
             (
                 "place-switch-tables-in-ram",
-                Value::Bool(true),
                 "Places switch-tables, some lookup tables and constants related to interrupt handling into RAM - resulting in better performance but slightly more RAM consumption.",
+                Value::Bool(true),
+                None
             ),
             (
                 "place-anon-in-ram",
-                Value::Bool(false),
                 "Places anonymous symbols into RAM - resulting in better performance at the cost of significant more RAM consumption. Best to be combined with `place-switch-tables-in-ram`.",
+                Value::Bool(false),
+                None
             ),
         ],
         true,

esp-ieee802154/build.rs

@@ -1,6 +1,6 @@
 use std::{env, path::PathBuf};
 
-use esp_config::{generate_config, Value};
+use esp_config::{generate_config, Validator, Value};
 
 fn main() {
     let out = PathBuf::from(env::var_os("OUT_DIR").unwrap());
@@ -11,8 +11,9 @@ fn main() {
         "esp_ieee802154",
         &[(
             "rx_queue_size",
-            Value::UnsignedInteger(50),
             "Size of the RX queue in frames",
+            Value::Integer(50),
+            Some(Validator::PositiveInteger),
         )],
         true,
     );

esp-wifi/build.rs

@@ -1,7 +1,7 @@
 use std::{error::Error, str::FromStr};
 
 use esp_build::assert_unique_used_features;
-use esp_config::{generate_config, Value};
+use esp_config::{generate_config, Validator, Value};
 use esp_metadata::{Chip, Config};
 
 fn main() -> Result<(), Box<dyn Error>> {
@@ -99,37 +99,134 @@ fn main() -> Result<(), Box<dyn Error>> {
     generate_config(
         "esp_wifi",
         &[
-            ("rx_queue_size", Value::UnsignedInteger(5), "Size of the RX queue in frames"),
-            ("tx_queue_size", Value::UnsignedInteger(3), "Size of the TX queue in frames"),
-            ("static_rx_buf_num", Value::UnsignedInteger(10), "WiFi static RX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("dynamic_rx_buf_num", Value::UnsignedInteger(32), "WiFi dynamic RX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("static_tx_buf_num", Value::UnsignedInteger(0), "WiFi static TX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("dynamic_tx_buf_num", Value::UnsignedInteger(32), "WiFi dynamic TX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("csi_enable", Value::Bool(false), "WiFi channel state information enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("ampdu_rx_enable", Value::Bool(true), "WiFi AMPDU RX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("ampdu_tx_enable", Value::Bool(true), "WiFi AMPDU TX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("amsdu_tx_enable", Value::Bool(false), "WiFi AMSDU TX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("rx_ba_win", Value::UnsignedInteger(6), "WiFi Block Ack RX window size. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)"),
-            ("max_burst_size", Value::UnsignedInteger(1), "See [smoltcp's documentation](https://docs.rs/smoltcp/0.10.0/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_burst_size)"),
+            ("rx_queue_size", "Size of the RX queue in frames", Value::Integer(5), Some(Validator::PositiveInteger)),
+            ("tx_queue_size", "Size of the TX queue in frames", Value::Integer(3), Some(Validator::PositiveInteger)),
+            (
+                "static_rx_buf_num",
+                "WiFi static RX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Integer(10),
+                None
+            ),
+            (
+                "dynamic_rx_buf_num",
+                "WiFi dynamic RX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Integer(32),
+                None
+            ),
+            (
+                "static_tx_buf_num",
+                "WiFi static TX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Integer(0),
+                None
+            ),
+            (
+                "dynamic_tx_buf_num",
+                "WiFi dynamic TX buffer number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Integer(32),
+                None
+            ),
+            (
+                "csi_enable",
+                "WiFi channel state information enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Bool(false),
+                None
+            ),
+            (
+                "ampdu_rx_enable",
+                "WiFi AMPDU RX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Bool(true),
+                None
+            ),
+            (
+                "ampdu_tx_enable",
+                "WiFi AMPDU TX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Bool(true),
+                None
+            ),
+            (
+                "amsdu_tx_enable",
+                "WiFi AMSDU TX feature enable flag. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Bool(false),
+                None
+            ),
+            (
+                "rx_ba_win",
+                "WiFi Block Ack RX window size. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv418wifi_init_config_t)",
+                Value::Integer(6),
+                None
+            ),
+            (
+                "max_burst_size",
+                "See [smoltcp's documentation](https://docs.rs/smoltcp/0.10.0/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_burst_size)",
+                Value::Integer(1),
+                None
+            ),
             (
                 "country_code",
-                Value::String("CN".to_owned()),
                 "Country code. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/wifi.html#wi-fi-country-code)",
+                Value::String("CN".to_owned()),
+                None
             ),
             (
                 "country_code_operating_class",
-                Value::UnsignedInteger(0),
                 "If not 0: Operating Class table number. See [ESP-IDF Programming Guide](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/wifi.html#wi-fi-country-code)",
+                Value::Integer(0),
+                None
+            ),
+            (
+                "mtu",
+                "MTU, see [smoltcp's documentation](https://docs.rs/smoltcp/0.10.0/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_transmission_unit)",
+                Value::Integer(1492),
+                None
+            ),
+            (
+                "tick_rate_hz",
+                "Tick rate of the internal task scheduler in hertz",
+                Value::Integer(100),
+                None
+            ),
+            (
+                "listen_interval",
+                "Interval for station to listen to beacon from AP. The unit of listen interval is one beacon interval. For example, if beacon interval is 100 ms and listen interval is 3, the interval for station to listen to beacon is 300 ms",
+                Value::Integer(3),
+                None
+            ),
+            (
+                "beacon_timeout",
+                "For Station, If the station does not receive a beacon frame from the connected SoftAP during the  inactive time, disconnect from SoftAP. Default 6s. Range 6-30",
+                Value::Integer(6),
+                None
+            ),
+            (
+                "ap_beacon_timeout",
+                "For SoftAP, If the SoftAP doesn't receive any data from the connected STA during inactive time, the SoftAP will force deauth the STA. Default is 300s",
+                Value::Integer(300),
+                None
+            ),
+            (
+                "failure_retry_cnt",
+                "Number of connection retries station will do before moving to next AP. scan_method should be set as WIFI_ALL_CHANNEL_SCAN to use this config. Note: Enabling this may cause connection time to increase incase best AP doesn't behave properly. Defaults to 1",
+                Value::Integer(1),
+                None
+            ),
+            (
+                "scan_method",
+                "0 = WIFI_FAST_SCAN, 1 = WIFI_ALL_CHANNEL_SCAN, defaults to 0",
+                Value::Integer(0),
+                None
+            ),
+            (
+                "dump_packets",
+                "Dump packets via an info log statement",
+                Value::Bool(false),
+                None
+            ),
+            (
+                "phy_enable_usb",
+                "Keeps USB running when using WiFi. This allows debugging and log messages via USB Serial JTAG. Turn off for best WiFi performance.",
+                Value::Bool(true),
+                None
             ),
-            ("mtu", Value::UnsignedInteger(1492), "MTU, see [smoltcp's documentation](https://docs.rs/smoltcp/0.10.0/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_transmission_unit)"),
-            ("tick_rate_hz", Value::UnsignedInteger(100), "Tick rate of the internal task scheduler in hertz"),
-            ("listen_interval", Value::UnsignedInteger(3), "Interval for station to listen to beacon from AP. The unit of listen interval is one beacon interval. For example, if beacon interval is 100 ms and listen interval is 3, the interval for station to listen to beacon is 300 ms"),
-            ("beacon_timeout", Value::UnsignedInteger(6), "For Station, If the station does not receive a beacon frame from the connected SoftAP during the  inactive time, disconnect from SoftAP. Default 6s. Range 6-30"),
-            ("ap_beacon_timeout", Value::UnsignedInteger(300), "For SoftAP, If the SoftAP doesn’t receive any data from the connected STA during inactive time, the SoftAP will force deauth the STA. Default is 300s"),
-            ("failure_retry_cnt", Value::UnsignedInteger(1), "Number of connection retries station will do before moving to next AP. scan_method should be set as WIFI_ALL_CHANNEL_SCAN to use this config. Note: Enabling this may cause connection time to increase incase best AP doesn't behave properly. Defaults to 1"),
-            ("scan_method", Value::UnsignedInteger(0), "0 = WIFI_FAST_SCAN, 1 = WIFI_ALL_CHANNEL_SCAN, defaults to 0"),
-            ("dump_packets", Value::Bool(false), "Dump packets via an info log statement"),
-            ("phy_enable_usb", Value::Bool(true), "Keeps USB running when using WiFi. This allows debugging and log messages via USB Serial JTAG. Turn off for best WiFi performance."),
         ],
         true
     );