Skip to content

Revert "Minimal(ish) changes needed to get the Pico Core working with Harp Regulator" #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Poofjunior merged 1 commit into from
Jul 17, 2025
Merged

Revert "Minimal(ish) changes needed to get the Pico Core working with Harp Regulator" #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 1 addition & 5 deletions examples/harp_c_app_example/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ add_definitions(-DGIT_HASH="${COMMIT_ID}") # Usable in source code.

# Specify USB Manufacturer and Product descriptions.
add_definitions(-DUSBD_MANUFACTURER="Allen Institute")
add_definitions(-DHARP_DEVICE_DESCRIPTION="Harp3226 | Fw5.0.0 | Harp Club Demo")
add_definitions(-DUSBD_PRODUCT="Example Device")

# PICO_SDK_PATH must be defined.
include(${PICO_SDK_PATH}/pico_sdk_init.cmake)
Expand All @@ -25,10 +25,6 @@ add_executable(${PROJECT_NAME}
src/main.cpp
)

pico_set_program_name(${PROJECT_NAME} "device.regulatordemo")
pico_set_program_description(${PROJECT_NAME} "Harp3226 | Fw5.0.0 | Harp Club Demo")
pico_set_program_version(${PROJECT_NAME} "5.0.0")

include_directories(inc)

target_link_libraries(${PROJECT_NAME} harp_c_app harp_sync pico_stdlib)
Expand Down
87 changes: 12 additions & 75 deletions examples/harp_c_app_example/src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,74 +8,47 @@
#include <cstdio> // for printf
#endif

#include <harp_core.h>
#include <hardware/flash.h>
#include <pico/bootrom.h>

// Create device name array.
const uint16_t who_am_i = 3226;
const uint16_t who_am_i = 1234;
const uint8_t hw_version_major = 1;
const uint8_t hw_version_minor = 0;
const uint8_t assembly_version = 2;
const uint8_t harp_version_major = 2;
const uint8_t harp_version_minor = 0;
const uint8_t fw_version_major = 5;
const uint8_t fw_version_major = 3;
const uint8_t fw_version_minor = 0;
const uint16_t serial_number = 0xCAFE;

// Harp App Register Setup.
const size_t reg_count = 2;

#define FIRMWARE_UPDATE_PICO_BOOTSEL 1

// Define register contents.
#pragma pack(push, 1)
struct app_regs_t
{
volatile uint32_t firmware_update_capabilities;
volatile uint32_t firmware_update_start;
volatile uint8_t test_byte; // app register 0
volatile uint32_t test_uint; // app register 1
} app_regs;
#pragma pack(pop)

static void handle_reset_command(msg_t& msg)
{
HarpCore::copy_msg_payload_to_register(msg);
if (app_regs.firmware_update_start == FIRMWARE_UPDATE_PICO_BOOTSEL)
{
HarpCore::send_harp_reply(WRITE, msg.header.address);

// Give controller a chance to gracefully close the serial port
for (int i = 0; i < 5; i++)
{
sleep_ms(100);
tud_task();
}

rom_reset_usb_boot(0, 0);
}
else
HarpCore::send_harp_reply(WRITE_ERROR, msg.header.address);

app_regs.firmware_update_start = 0;
}

// Define register "specs."
RegSpecs app_reg_specs[reg_count]
{
{(uint8_t*)&app_regs.firmware_update_capabilities, sizeof(app_regs.firmware_update_capabilities), U32},
{(uint8_t*)&app_regs.firmware_update_start, sizeof(app_regs.firmware_update_start), U32},
{(uint8_t*)&app_regs.test_byte, sizeof(app_regs.test_byte), U8},
{(uint8_t*)&app_regs.test_uint, sizeof(app_regs.test_uint), U32}
};

// Define register read-and-write handler functions.
RegFnPair reg_handler_fns[reg_count]
{
{&HarpCore::read_reg_generic, &HarpCore::write_to_read_only_reg_error},
{&HarpCore::read_reg_generic, &handle_reset_command}, // write-only
{&HarpCore::read_reg_generic, &HarpCore::write_reg_generic},
{&HarpCore::read_reg_generic, &HarpCore::write_to_read_only_reg_error}
};

void app_reset()
{
app_regs.firmware_update_capabilities = FIRMWARE_UPDATE_PICO_BOOTSEL;
app_regs.firmware_update_start = 0;
app_regs.test_byte = 0;
app_regs.test_uint = 0;
}

void update_app_state()
Expand All @@ -84,46 +57,14 @@ void update_app_state()
// If app registers update their states outside the read/write handler
// functions, update them here.
// (Called inside run() function.)
gpio_put(PICO_DEFAULT_LED_PIN, (time_us_64() / 1000000) & 1);
}

//DJM: This should ideally be default behavior within the Pico Core if the user doesn't explicitly provide a serial number
uint16_t get_serial_number()
{
static_assert(sizeof(pico_unique_board_id_t) == sizeof(uint64_t));
union
{
pico_unique_board_id_t id_array;
uint64_t id64;
} board_id;
//TODO: This isn't working for whatever reason, I just get a bunch of 0's
// For some reason the constructor isn't running. I noticed multiple copies of unique_id.obj are being built, maybe that has something to do with it?
//pico_get_unique_board_id(&board_id.id_array);

// Using flash_get_unique_id directly as a workaround for now. This is not ideal as it doesn't properly handle some edge cases or the RP2350.
flash_get_unique_id(&board_id.id_array.id[0]);

//TODO: 2 bytes is super very bad for a unique identifier.
// Ideally Harp Regulator wants the first or last two bytes of the serial number as it's used to match the serial number seen in PICOBOOT (which matches pico_get_unique_board_id)
// and Harp Regulator uses a prefix/suffix match to handle the difference in sizes.
//
// Be mindful of whether we prefer the upper or lower 16 bits, the Pico SDK used to use the wrong end of the flash ID when it exceeded 8 bytes and it resulted
// in some flash chips with 16 byte IDs all having the same unique board ID. We likely want to tailor this to whatever flash chips are generally used with the Pico core
// (or we should support a longer serial number somehow. Maybe if the client sends a READ U64 it replies with the full thing?)
// https://github.com/raspberrypi/pico-sdk/issues/1641
// https://github.com/raspberrypi/pico-sdk/issues/1132#issuecomment-2285719484
//
// With the official Raspberry Pi Pico boards, it seems my 16 least-significant bits are the ones with the most entropy.
// (The upper 16 bits are actually identical on the two boards I checked.)
return (uint16_t)board_id.id_array.id[6] << 8 | (uint16_t)board_id.id_array.id[7];
}

// Create Harp App.
HarpCApp& app = HarpCApp::init(who_am_i, hw_version_major, hw_version_minor,
assembly_version,
harp_version_major, harp_version_minor,
fw_version_major, fw_version_minor,
get_serial_number(), "Example C App",
serial_number, "Example C App",
(const uint8_t*)GIT_HASH, // in CMakeLists.txt.
&app_regs, app_reg_specs,
reg_handler_fns, reg_count, update_app_state,
Expand All @@ -132,10 +73,6 @@ HarpCApp& app = HarpCApp::init(who_am_i, hw_version_major, hw_version_minor,
// Core0 main.
int main()
{
gpio_init(PICO_DEFAULT_LED_PIN);
gpio_set_dir(PICO_DEFAULT_LED_PIN, GPIO_OUT);
app_reset(); //DJM: Should the core call this to ensure everything starts out in some neutral state?

// Init Synchronizer.
HarpSynchronizer& sync = HarpSynchronizer::init(uart1, 5);
app.set_synchronizer(&sync);
Expand Down
18 changes: 9 additions & 9 deletions firmware/src/usb_descriptors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,15 @@

// FIXME: can we get these into a header file?
// Override "Pico". Do this before including pico-related dependencies.
#ifndef HARP_DEVICE_DESCRIPTION
#warning "Harp device description is not configured!"
#define HARP_DEVICE_DESCRIPTION "Harp0|Unnamed Harp Device"
#ifndef USBD_PRODUCT
#define USBD_PRODUCT "Harp Device"
#endif

#include "tusb.h"
//#include "pico/stdio_usb/reset_interface.h"
#include "pico/unique_id.h"

//TODO: Is there any actual downside to just using the max value here?
#define USBD_DESC_STR_MAX (127) // Override default of 20 (max 127).
#define USBD_DESC_STR_MAX (64) // Override default of 20 (max 127).

#ifndef USBD_VID
#define USBD_VID (0x2E8A) // Raspberry Pi
Expand Down Expand Up @@ -89,8 +87,9 @@

#define USBD_STR_0 (0x00)
#define USBD_STR_MANUF (0x01)
#define USBD_STR_HARP_DESCRIPTION (0x02)
#define USBD_STR_PRODUCT (0x02)
#define USBD_STR_SERIAL (0x03)
#define USBD_STR_CDC (0x04)
#define USBD_STR_RPI_RESET (0x05)

// Note: descriptors returned from callbacks must exist long enough for transfer to complete
Expand All @@ -107,7 +106,7 @@ static const tusb_desc_device_t usbd_desc_device = {
.idProduct = USBD_PID,
.bcdDevice = 0x0100,
.iManufacturer = USBD_STR_MANUF,
.iProduct = USBD_STR_HARP_DESCRIPTION,
.iProduct = USBD_STR_PRODUCT,
.iSerialNumber = USBD_STR_SERIAL,
.bNumConfigurations = 1,
};
Expand All @@ -120,7 +119,7 @@ static const uint8_t usbd_desc_cfg[USBD_DESC_LEN] = {
TUD_CONFIG_DESCRIPTOR(1, USBD_ITF_MAX, USBD_STR_0, USBD_DESC_LEN,
USBD_CONFIGURATION_DESCRIPTOR_ATTRIBUTE, USBD_MAX_POWER_MA),

TUD_CDC_DESCRIPTOR(USBD_ITF_CDC, USBD_STR_HARP_DESCRIPTION, USBD_CDC_EP_CMD,
TUD_CDC_DESCRIPTOR(USBD_ITF_CDC, USBD_STR_CDC, USBD_CDC_EP_CMD,
USBD_CDC_CMD_MAX_SIZE, USBD_CDC_EP_OUT, USBD_CDC_EP_IN, USBD_CDC_IN_OUT_MAX_SIZE),

#if PICO_STDIO_USB_ENABLE_RESET_VIA_VENDOR_INTERFACE
Expand All @@ -132,8 +131,9 @@ static char usbd_serial_str[PICO_UNIQUE_BOARD_ID_SIZE_BYTES * 2 + 1];

static const char *const usbd_desc_str[] = {
[USBD_STR_MANUF] = USBD_MANUFACTURER,
[USBD_STR_HARP_DESCRIPTION] = HARP_DEVICE_DESCRIPTION,
[USBD_STR_PRODUCT] = USBD_PRODUCT,
[USBD_STR_SERIAL] = usbd_serial_str,
[USBD_STR_CDC] = "Board CDC",
#if PICO_STDIO_USB_ENABLE_RESET_VIA_VENDOR_INTERFACE
[USBD_STR_RPI_RESET] = "Reset",
#endif
Expand Down