✨ (minor) Usb enumeration #92

PhazonicRidley · 2026-01-30T00:17:51Z

Implemented a USB enumerator that can be used in various projects. This is meant as a default and reference implementation of an enumerator using the libhal USB system.

kammce

Please rebase your project and fix your merge conflicts. Also, we moved the libhal-util code into v5 in preperation for v6 which will use libhal-v5. So we can remove the include files within the include directory and only keep the ones within v5/*

.github/workflows/ci.yml

include/libhal-util/usb/descriptors.hpp

include/libhal-util/usb/endpoints.hpp

For devices that use the libhal usb interface high level class. Handles configuration at the device and usb configuration level Handles multiple configurations and multiple interfaces.

Simulated host by sending enumerator requests and having it respond. Correct payloads were verified

Changed usb.hpp to be a shorthand for including all usb utils

For devices that use the libhal usb interface high level class. Handles configuration at the device and usb configuration level Handles multiple configurations and multiple interfaces.

Simulated host by sending enumerator requests and having it respond. Correct payloads were verified

Changed usb.hpp to be a shorthand for including all usb utils

kammce

I haven't reviewed everything. I'm posting what I have now because I'm not sure if github will lose my info.

.claude/settings.local.json

v5/include/libhal-util/mock/usb.hpp

v5/include/libhal-util/usb/enumerator.hpp

kammce · 2026-02-09T23:14:01Z

v5/include/libhal-util/usb/descriptors.hpp

+  }
+
+  std::u16string_view name;
+  std::pmr::vector<strong_ptr<interface>> interfaces;


Consider using hal::allocated_buffer<strong_ptr<interface>> instead of vector. We don't need to resize the array after construction, and thus all of its code needed to handle pushing and resizing can be eliminated. This way we pay for only what we needed which is a pointer, a length, and the allocator that created.

#include <libhal/allocated_buffer.hpp>

It was meant for just buffers but its usable for any type. (maybe I should make an alias for it thats dynamic_array).

I just realized a concern I have with std::pmr::vector. And apologies for bringing this up on late notice, but I think we should design with the intention that our allocator is monotonic and thus doesn't free memory. Meaning, if we make a copy of this configuration and destroy the old one, we don't get that memory back. That makes copying this type problematic. I don't have an answer. We may want to talk over voice chat to discuss this further.

v5/include/libhal-util/usb/enumerator.hpp

kammce · 2026-02-10T00:34:41Z

v5/include/libhal-util/usb/enumerator.hpp

+    }
+  }
+
+  void enumerate()


This is purely a suggestion but I think we might benefit from this rename:

Suggested change

void enumerate()

void enumerate_sync_prototype0()

Why? We can provide this temporary API that we plan to deprecate in the future. This way we can work with the experimental stuff while preventing the API from being broken. This will allow us to publish the code and leave space for a better implementation in the future. Because making this async may require a breaking API change. Or a new API called enumerate2().

How about enumerate_sync()

No, because I don't think this API is going to stick around. I don't know if enumeration_sync() is usable without an RTOS. I'm hoping we don't need an RTOS for our APIs.

Added required copyright notices Changed over from span_eq to ranges::equal and removed function Renamed truncated constant names to their full names Changed mock interface's packed array to use a predefined constant instead of a magic number Changed headers in the same library to be relative paths Factored out ctor arguments for device, config, and enumeration Enforced endianess with device descriptor Changed ref getters to return by value and add setters where needed Documented make_sub_scatter_array and changed return type to a struct for clarity Hardcoded string indexes for device descriptors, removed the ablity to set a start for string descriptors

kammce

This is a partial review of the changes you've made. Things are looking really good. I plan to dig into the details of the enumerator next. Thank you for your effort on this!

kammce · 2026-02-11T15:40:25Z

v5/include/libhal-util/usb/descriptors.hpp

 #pragma once

 #include <array>
+#include <libhal/allocated_buffer.hpp>


Move this with the other libhal includes. Keep it away from the C++ headers.

kammce · 2026-02-11T15:43:14Z

v5/include/libhal-util/usb/descriptors.hpp

+  }
+
+  std::u16string_view name;
+  std::pmr::vector<strong_ptr<interface>> interfaces;


I just realized a concern I have with std::pmr::vector. And apologies for bringing this up on late notice, but I think we should design with the intention that our allocator is monotonic and thus doesn't free memory. Meaning, if we make a copy of this configuration and destroy the old one, we don't get that memory back. That makes copying this type problematic. I don't have an answer. We may want to talk over voice chat to discuss this further.

kammce · 2026-02-11T15:50:27Z

v5/include/libhal-util/usb/enumerator.hpp

    if (determine_standard_request(req) ==
          standard_request_types::get_descriptor &&
-        static_cast<descriptor_type>((req.value() & 0xFF << 8) >> 8) ==
+        static_cast<descriptor_type>(req.value() >> 8 & 0xFF) ==


Suggestion: I see this value() >> 8 & 0xFF a few times in the code? Maybe this warrants a free function or class method for getting this specific value from the request?

kammce · 2026-02-11T15:53:43Z

v5/tests/usb.test.cpp

@@ -74,11 +74,17 @@ boost::ut::suite<"enumeration_test"> enumeration_test = [] {
    make_strong_ptr<std::array<configuration, 1>>(&pool, conf_arr);


I'm a bit concerned that this results in a copy. It should be a move. I'm wondering if we should make configuration a move-only object.

kammce

alright, this is the review of the enumerator it self. There is a lot of extra code hanging around that needs some cleanup. And I think we should think very critically about errors and how we expect developers to handle them. No error should come without a means to handle it. And I think in most cases, the exceptions can be eliminated entirely.

kammce · 2026-02-11T16:05:53Z

v5/include/libhal-util/usb/enumerator.hpp

+      //
+      // for (auto const& el : raw_req) {
+      //
+      // }
+      //


Suggested change

//

// for (auto const& el : raw_req) {

//

// }

//

kammce · 2026-02-11T16:08:46Z

v5/include/libhal-util/usb/enumerator.hpp

+
+      // TODO: Handle exception
+      handle_standard_device_request(req);
+      // m_ctrl_ep->write({});  // Send ZLP to complete Data


Suggested change

// m_ctrl_ep->write({}); // Send ZLP to complete Data

Was that still needed?

kammce · 2026-02-11T16:10:53Z

v5/include/libhal-util/usb/enumerator.hpp

+    } while (!finished_enumeration);
+  }
+
+  [[nodiscard]] configuration& get_active_configuration()


Is this meant to be public? And if so, what is the expected use case for devs?

kammce · 2026-02-11T16:11:45Z

v5/include/libhal-util/usb/enumerator.hpp

+    while (!m_has_setup_packet) {
+      continue;
+    }


Any reason to not just return from this point? Why must we halt until a setup package comes in?

kammce · 2026-02-11T16:13:36Z

v5/include/libhal-util/usb/enumerator.hpp

+          standard_request_types::get_descriptor &&
+        static_cast<descriptor_type>(req.value() >> 8 & 0xFF) ==
+          descriptor_type::string) {
+      handle_str_descriptors(req.value() & 0xFF, req.length() > 2);


This API accepts a length as the second parameter, but you've only passed in a bool which is true/false based on the request length. Is that what you wanted?

kammce · 2026-02-11T17:05:35Z

v5/include/libhal-util/usb/enumerator.hpp

+          auto s_hdr =
+            std::to_array({ static_cast<byte>(4),
+                            static_cast<byte>(descriptor_type::string) });
+          auto lang = setup_packet::to_le_u16(m_lang_str);
+          auto scatter_arr_pair = make_scatter_bytes(s_hdr, lang);


Maybe this should be its own descriptor to manage all of this?

kammce · 2026-02-11T20:41:48Z

v5/include/libhal-util/usb/enumerator.hpp

+    }
+  }
+
+  void handle_str_descriptors(u8 const target_idx, u16 p_len)


Suggested change

void handle_str_descriptors(u8 const target_idx, u16 p_len)

void handle_str_descriptors(u8 target_idx, u16 p_len)

No need for const here.

kammce · 2026-02-11T20:57:34Z

v5/include/libhal-util/usb/enumerator.hpp

+    std::mbstate_t state{};
+    for (wchar_t const wc : sv) {
+      std::array<char, 1024> tmp;
+      size_t len = std::wcrtomb(tmp.data(), wc, &state);
+      if (len == static_cast<size_t>(-1)) {
+
+        continue;
+      }
+
+      for (size_t i = 0; i < len; i++) {
+      }
+    }


What is the intention of this code?

kammce · 2026-02-11T20:58:34Z

v5/include/libhal-util/usb/enumerator.hpp

+    auto const conf_sv_span = hal::as_bytes(opt_conf_sv.value());
+    auto desc_len = static_cast<hal::byte>((conf_sv_span.size() + 2));
+
+    auto hdr_arr = std::to_array(
+      { desc_len, static_cast<hal::byte>(descriptor_type::string) });
+
+    auto scatter_arr_pair =
+      make_sub_scatter_bytes(p_len, hdr_arr, conf_sv_span);
+
+    auto p = scatter_span<byte const>(scatter_arr_pair.spans)
+               .first(scatter_arr_pair.count);


Feels like a string_descriptor like thing would be useful. One that provides sub scatter spans based on length.

kammce · 2026-02-11T21:02:13Z

v5/include/libhal-util/usb/enumerator.hpp

+      if (!req_handled) {
+        safe_throw(hal::argument_out_of_domain(this));
+      }


What should the caller of this function do when they get this exception? This is more of an issue regarding the host. And if so, is reporting this to the caller important or should the enumerator handle it itself. Like using STALL.

PhazonicRidley changed the title ~~✨ Usb enumeration~~ ✨ (minor) Usb enumeration Jan 30, 2026

PhazonicRidley force-pushed the usb-enumeration branch from d2cc07b to c8f1daa Compare January 30, 2026 00:27

PhazonicRidley marked this pull request as draft February 1, 2026 05:19

PhazonicRidley force-pushed the usb-enumeration branch 2 times, most recently from 780ef28 to f41be54 Compare February 5, 2026 20:57

kammce self-requested a review February 7, 2026 22:59

kammce requested changes Feb 8, 2026

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

include/libhal-util/usb/descriptors.hpp Outdated Show resolved Hide resolved

include/libhal-util/usb/endpoints.hpp Outdated Show resolved Hide resolved

PhazonicRidley force-pushed the usb-enumeration branch 3 times, most recently from 6b474f8 to c9c62f1 Compare February 9, 2026 19:10

PhazonicRidley marked this pull request as ready for review February 10, 2026 00:26

PhazonicRidley requested a review from kammce February 10, 2026 00:26

PhazonicRidley and others added 17 commits February 9, 2026 16:28

✨ feat(usb_enumeration): Made an enumerator

ce7b663

For devices that use the libhal usb interface high level class. Handles configuration at the device and usb configuration level Handles multiple configurations and multiple interfaces.

🏷️ Updated usb utils to use libhal 4.18.0+ APIs

e655173

✅ Tested enumerator

4cf652b

Simulated host by sending enumerator requests and having it respond. Correct payloads were verified

♻️ Moved endpoint utils to their own file

767b305

Changed usb.hpp to be a shorthand for including all usb utils

🐛 Enumerator wait loop got optmized out, volatile prevents this

67a1e4c

🐛 Fixed logic to have enumeration work on real hardware

033e5d7

📝 Document descriptors and enumerator class

1a03390

♻️ Move source code to v5/ directory (libhal#82)

2bef819

✨ feat(usb_enumeration): Made an enumerator

a89d57a

For devices that use the libhal usb interface high level class. Handles configuration at the device and usb configuration level Handles multiple configurations and multiple interfaces.

✅ Tested enumerator

b1c0e04

Simulated host by sending enumerator requests and having it respond. Correct payloads were verified

♻️ Moved endpoint utils to their own file

778857c

Changed usb.hpp to be a shorthand for including all usb utils

🐛 Enumerator wait loop got optmized out, volatile prevents this

6677c55

🐛 Fixed logic to have enumeration work on real hardware

c04d212

♻️ Refactored USB utils into their own folder and namespace

fd3074d

🧑‍💻 Updated to support new API for setup_packet in libhal

70b0405

🧑‍💻 Updated to support new API for setup_packet in libhal

50ebee4

🚚 Restructured files and APIs to align with latest libhal

ff91bbf

PhazonicRidley force-pushed the usb-enumeration branch from c9c62f1 to ff91bbf Compare February 10, 2026 00:29

kammce requested changes Feb 10, 2026

View reviewed changes

kammce requested changes Feb 11, 2026

View reviewed changes

		@@ -74,11 +74,17 @@ boost::ut::suite<"enumeration_test"> enumeration_test = [] {
		make_strong_ptr<std::array<configuration, 1>>(&pool, conf_arr);

	void handle_str_descriptors(u8 const target_idx, u16 p_len)
	void handle_str_descriptors(u8 target_idx, u16 p_len)

✨ (minor) Usb enumeration #92

Are you sure you want to change the base?

✨ (minor) Usb enumeration #92

Conversation

PhazonicRidley commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kammce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kammce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kammce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kammce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PhazonicRidley commented Jan 30, 2026 •

edited

Loading