The core should support "active," "standby," and "speed" modes. These should be pretty simple to implement.
Protocol Details

When requesting a write, for example to a core register, the instruction is completed but the returned message is of 'read' instead of 'write' type.

As-is, the cable cannot be removed from jack without causing the device to reset.

This is probably not a feature we care about, but it's worth noting here.

There are a few avenues to updating the Harp reply routine, all of which could offer significant performance benefits.

First, Tinyusb supports a multi-byte write tud_cdc_write that's likely to be faster than just calling tud_cdc_write_char. (We currently write chars one-at-a-time such that we can also calculate the checksum.)

However, if we know where the underlying data is being transferred, we may be able to do a DMA transfer to its final location while also computing the checksum automatically. (See RP2040 datasheet 2.5.5.2.)

It's worth investigating if we can speed up this function at all.

The list of RegSpecs is per-register supplementary information (example) needed to dispatch a Harp reply. This information could almost be generated with a compile-time LUT (example) if only we could iterate through the fields of a struct.

Enter Boost PFR (also magic_get), a library that lets you iterate over struct fields at compile time (example).

Reasons to implement this:

• It simplifies user-facing interface for creating a harp device

Reasons not to do this:

• It makes compilation across different setups more difficult
• It increases code size significantly
• it produces confusing errors for new users.
• the implementation becomes more complicated for inferring some sort of "composite" payload type where the payload is some sort of struct of mixed fields.

Details

We would need a helper function to infer the register type from the register at compile time. This can be done with function overloading and constexpr.

constexpr reg_type_t to_reg_type(uint8_t value) {return U8};
constexpr reg_type_t to_reg_type(int8_t value) {return S8};
constexpr reg_type_t to_reg_type(uint16_t value) {return U16};
constexpr reg_type_t to_reg_type(int16_t value) {return S16};
constexpr reg_type_t to_reg_type(uint32_t value) {return U32};
constexpr reg_type_t to_reg_type(int32_t value) {return S32};
constexpr reg_type_t to_reg_type(uint64_t value) {return U64};
constexpr reg_type_t to_reg_type(int64_t value) {return S64};
constexpr reg_type_t to_reg_type(float value) {return Float};

Harp devices must support the ALIVE_EN bit setting in the R_OPERATION_CTRL register.
Protocol Details

The implementation of the DUMP bit in R_OPERATION_CTRL is to serialize and dispatch all registers (app and core). The current implementation has no knowledge of any "App" registers and only sends the harp core registers.

Since there is only one harp instance, it would useful to make the public-facing API consistent. (This also makes it easy to write functions that attach callbacks to HarpCore static member functions before the HarpCore has been instantiated.)

In our local copy of usb_descriptors.c, it is not sufficient to set
#define PICO_STDIO_USB_ENABLE_RESET_VIA_VENDOR_INTERFACE (1).

What I'm seeing by setting the #define statement is that lsusb will show that a Reset interface descriptor does indeed exist, but the device hangs when we try to do anything with it.

That's most likely(?) because the pico-sdk's reset_interface.c is not being populated if we link to tinyusb_device in our CMakeLists.txt. We may need to make a local copy of the callback functions if we want to be able to use picotool (i.e: to reset the device from the PC without physically touching the BOOTSEL button).

Paste of lsusb output with Reset interface descriptor info shown below:

    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        2
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol      1 
      iInterface              5 Reset

References

• Possibly related: No way to use USB serial when tinyUSB library included

Writing a 1 to the DUMP bit field in the R_OPERATION_CTRL register is supposed to send out one harp reply message per register, not one harp reply with a payload containing all register contents. This nuance isn't really captured in the current docs, so it's worth clearing that up too.

As is, there is no clearly-defined protocol transaction to put the device into bootloader mode to accept new firmware.

For production purposes, it would be extremely valuable to be able to configure the device to accept a firmware device such that we don't need to physically dig every device instance out of every setup to update firmware.

• parse messages
• dispatch messages to the appropriate callback function.
• Handle replying to read requests.
• time keeping/updating from Harp Message SECONDS and MICROSECONDS register writes.
• populating reg name field on initialization.
• parse harp messages
• send harp replies (read reply, write reply, event reply)
• handle time keeping/updating from clock synchronizer.
• Implement key register flag settings.
• enable inheritance in a way that extends original register set.
• save non-volatile register data to eeprom
• check checksum

Register flag settings:

• R_OPERATION_CTRL: DUMP
• R_OPERATION_CTRL: MUTE_RPL
• R_OPERATION_CTRL: VISUALEN

Other:

• Easier way to debug. printf should be init on uart.
• queue outgoing harp messages.
• queue incoming harp messages.

It's likely that this repo will be submoduled, so it's preferrable that we remove the pico-sdk as a submodule and instead have people define PICO_SDK_PATH.

Firmware README instructions will need to be updated as well.

All of our USB interactions should be handled via TinyUSB as a CDC device.

Removing pico-sdk-specific calls in favor of TinyUSB is a key step in making this harp core useable on other platforms. In this case, the range would broaden to include any microcontroller that supports TinyUSB.

• TinyUSB Reference:
  • cdc_device.h
  • tusb_config.h for cdc devices
• CDC example: https://gist.github.com/jeremyherbert/45fbcd32e6bd187375459ad6402dd0bb#file-main-c-L188
• and another: https://git.lain.faith/sys64738/DragonProbe/commit/270c127bca167c337b8675638af6b518cb83709e?style=unified&whitespace=ignore-change#diff-13b6de1453ab7ea0288ac8fc59995d480fc5327e
• Pico Implementation

We will need a TinyUSB CDC config, but we can reuse the existing ones for stdio.

• tusb_config.h
• stdio_usb_descriptors.c
• some compilation notes on this.

Since we now have a way to establish the state of the connection between PC and device (ie DTR virtual pin) we should consider flushing the outgoing buffer to prevent sporadic messages from being sent when a new connection is established.

There is a race condition where it is possible for the reported Timestamp seconds and microseconds to reference a different offset and therefore report an erroneous value. This happens if the timestamp offset is updated via interrupt while the timestamp is being added into the reply message.

The fix would be to make sure we reference the same offset when we retrieve the time and compute it to update the timestamp registers and then read them. It really just boils down to making sure that the local offset value (offset_us_64_) is either a monotonic read or read from a double-buffer.
https://github.com/AllenNeuralDynamics/harp.core.rp2040/blob/main/firmware/inc/harp_core.h#L351

It's worth considering inlining this function.

We should expose some override-able fields that will give us the ability to define USB-related device descriptor data.
This info is especially useful for figuring out which device to connect to among a sea of devices. This info would also be super useful to create a tool to update the firmware of a particular device.

These should include:

• vendor id
• product id
• device description

To actually use valid Vendor/Product IDs, it's worth requesting a PID from https://pid.codes/

That way, our setups can fit in an ecosystem of everyday products.

According to the protocol, controller-to-device messages do not include timestamps. If the protocol were to add timestamps, the device could interpret these messages as "do-this-read/write@timestamp," i.e: a scheduled register read or write at a future time.

To handle scheduled/read events, we would need to incorporate a way for handling a schedule of messages. A priority queue would be a great choice here, the ETL has an existing data structure for this, and I've written a snippet of source code in the past to demonstrate how this could work without dynamic memory allocation and a low memory footprint.

It's worth mentioning that messages handled at a later time need to be copied out of the receive buffer. Memcpy would probably be fine here, but we could even borrow a DMA channel to move the data without blocking.

Including and linking pico_mem_ops will replace memcpy with an arm-specific implementation. We may save a few cycles in the core and in the lick detector sequence

https://forums.raspberrypi.com/viewtopic.php?t=311902

The Pico-SDK has a slightly slower implementation of reading the 64-bit timestamp registers, whereas reading the latching register directly (with interrupts disabled) is twice as fast.

inline uint64_t time_us_64_fast()
{
    uint32_t status = save_and_disable_interrupts();
    uint64_t time_us = timer_hw->timelr; // Locks time until we read TIMEHR.
    time_us |= (uint64_t(timer_hw->timerhr) << 32);
    restore_interrupts(status);
    return time_us;
}

And if you can guarantee that you don't call this from interrupts, you could omit the calls to save-and-restore interrupts too.

inline uint64_t time_us_64_fast_unsafe()
{
    uint64_t time_us = timer_hw->timelr; // Locks time until we read TIMEHR.
    time_us |= (uint64_t(timer_hw->timerhr) << 32);
    return time_us;
}