Skip to content

feat(unitree): forward aes_128_key to WebRTC driver for G1 firmware >=1.5.1#2117

Open
mihai-chiorean wants to merge 9 commits into
dimensionalOS:mainfrom
mihai-chiorean:feat/forward-aes-128-key
Open

feat(unitree): forward aes_128_key to WebRTC driver for G1 firmware >=1.5.1#2117
mihai-chiorean wants to merge 9 commits into
dimensionalOS:mainfrom
mihai-chiorean:feat/forward-aes-128-key

Conversation

@mihai-chiorean
Copy link
Copy Markdown

Motivation

Unitree G1 firmware 1.5.1 introduced a data2=3 WebRTC handshake that requires a per-device AES-128 key. Without it the connection fails inside unitree_webrtc_connect with RSA key format is not supported (pycryptodome trying to parse ciphertext bytes as an RSA key).

The per-device key is fetched from Unitree's cloud once via unitree-fetch-aes-key --email YOU --sn <serial> and then cached locally. The upstream driver legion1581/unitree_webrtc_connect@v2.1.1 accepts it through an aes_128_key= kwarg on UnitreeWebRTCConnection. The dimos wrapper at dimos/robot/unitree/connection.py never forwarded it, so anyone on G1 firmware >=1.5.1 cannot use the dimos Unitree integration today.

Change

UnitreeWebRTCConnection.__init__ now accepts an optional aes_128_key: str | None = None kwarg and falls back to the UNITREE_AES_128_KEY environment variable. The value is forwarded to the underlying LegionConnection only when non-empty, so the call is byte-identical to the previous behaviour when unset.

def __init__(self, ip: str, mode: str = "ai", aes_128_key: str | None = None) -> None:
    ...
    if aes_128_key is None:
        aes_128_key = os.environ.get("UNITREE_AES_128_KEY")
    extra: dict[str, Any] = {"aes_128_key": aes_128_key} if aes_128_key else {}
    self.conn = LegionConnection(WebRTCConnectionMethod.LocalSTA, ip=self.ip, **extra)

Verification

Tested on a Unitree G1 EDU+ (model string G1_Edu+) at firmware 1.5.1.1. With UNITREE_AES_128_KEY exported in the shell, dimos run unitree-g1-basic proceeds past the WebRTC handshake on 192.168.123.161:9991 and the robot accepts motion-switcher / wireless-controller commands as before. Without the env var, the same command reproduces RSA key format is not supported and the handshake never completes — same behaviour as main.

No breaking change

The kwarg is optional and defaults to None. The env-var lookup is opt-in (only consulted when the kwarg is None). Callers on G1 firmware <1.5.1 and all Go2 robots see no behavioural change — the **extra is empty and the LegionConnection call is byte-identical to the old one.

Optional follow-up (not in this PR)

pyproject.toml currently pins unitree-webrtc-connect-leshy>=2.0.7. The aes_128_key kwarg + the _decrypt_data1_v3 path that consumes it landed in legion1581/unitree_webrtc_connect@v2.1.1. If unitree-webrtc-connect-leshy is a downstream rebuild of that repo and is already on >=2.1.1, no change is needed and this PR is enough on its own. If it's still tracking 2.0.x, the dep will also need to be bumped (e.g. via [tool.uv.sources] pointing at git+https://github.com/legion1581/unitree_webrtc_connect.git@v2.1.1). Happy to follow up in a separate PR if maintainers confirm the situation.

…=1.5.1

Unitree G1 firmware 1.5.1 added a data2=3 WebRTC handshake that requires
a per-device AES-128 key (fetched from Unitree cloud once via
`unitree-fetch-aes-key --email YOU --sn <serial>`). The upstream
`legion1581/unitree_webrtc_connect@v2.1.1` accepts this via the
`aes_128_key=` kwarg; without it the handshake fails with
`RSA key format is not supported` from pycryptodome reading ciphertext
bytes.

`UnitreeWebRTCConnection.__init__` now accepts an optional
`aes_128_key` kwarg and falls back to the `UNITREE_AES_128_KEY`
environment variable so existing call sites do not need to change.
When the value is unset the call to `LegionConnection` is byte-identical
to the previous behaviour, so this is a no-op for G1 firmware <1.5.1
and for Go2.

Verified against a Unitree G1 EDU+ on firmware 1.5.1.1 via
`dimos run unitree-g1-basic` - the WebRTC handshake on
192.168.123.161:9991 succeeds with UNITREE_AES_128_KEY exported and
reproduces the `RSA key format is not supported` failure without it.
@leshy
Copy link
Copy Markdown
Member

leshy commented May 17, 2026

reason for legion client fork is that unitree-webrtc-connect-leshy had a more efficinet lidar data parser and we had some issues with aiortc package pinning and had to fork it as well

legion1581/unitree_webrtc_connect@master...leshy:unitree_webrtc_connect:master
I assume lidar stuff is merged in the legion lib and all is resolved now

for FDEs (or anyone else here in the thread:) - we need to validate this on current g1s/go2s (ensure my lidar changes are now merged to legion client, transition to legion lib, test on our robots, make sure installation works, merge this)

Comment thread dimos/robot/unitree/connection.py
@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/robot/unitree/connection.py 33.33% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@natcl
Copy link
Copy Markdown

natcl commented May 17, 2026

had a more efficinet lidar data parser

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

Address PR review feedback from dimensionalOS#2117:

1. G1Config and G1HighLevelWebRtcConfig now expose 'aes_128_key' as an
   optional declarative config field, forwarded to UnitreeWebRTCConnection.
   Users on G1 firmware >= 1.5.1 can now set it via blueprint config
   without resorting to the UNITREE_AES_128_KEY env var. The env-var
   fallback in UnitreeWebRTCConnection.__init__ is preserved as-is.

2. Adds dimos/robot/unitree/test_connection.py with five test cases that
   cover the kwarg-forwarding logic without hardware:
   - default behaviour: no kwarg, no env → aes_128_key not forwarded
     (byte-identical to pre-PR call)
   - explicit kwarg is forwarded verbatim
   - env-var fallback when kwarg is None
   - explicit kwarg beats env-var (precedence)
   - empty-string kwarg is treated as 'no key' (truthiness guard)

Tests are pure-Python: LegionConnection is mocked, .connect() is patched
to a no-op. Default pytest selector (-m 'not tool ...') will run them.
Comment thread dimos/robot/unitree/connection.py Outdated
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@mihai-chiorean
Copy link
Copy Markdown
Author

I Was at a hackathon, but I'll get this sorted out shortly.

@spomichter
Copy link
Copy Markdown
Contributor

Thanks! @ruthwikdasyam or @Krishna can assist with any testing here

Mirrors the G1 wiring from this PR so Go2 firmware >= 1.1.15 (same data2=3
WebRTC handshake change) can pass the per-device AES-128 key. Adds the
field to Go2's ConnectionConfig, accepts it as a kwarg on make_connection,
and forwards it to UnitreeWebRTCConnection in the webrtc branch. Other
branches (replay, mujoco, dimsim) are unaffected.

Also fixes the test docstring greptile flagged after the 'if not
aes_128_key' guard landed: the test asserting empty-string + unset env →
no forwarding had a misleading docstring claiming the kwarg was 'treated
as no key'. Rewrites the docstring and adds a sibling test covering the
case that actually motivated the guard fix: empty-string kwarg + set env
→ env value wins (the YAML/JSON 'aes_128_key: ""' regression case).

Addresses natcl's review comment on PR dimensionalOS#2117.
@mihai-chiorean
Copy link
Copy Markdown
Author

Force-pushed 6bff60d44 addressing the open review items:

  • @natcl — Go2 firmware ≥1.1.15 coverage added. aes_128_key now flows through go2/connection.py (ConnectionConfig field → make_connection(..., aes_128_key=...)UnitreeWebRTCConnection) mirroring the G1 pattern, plus through go2/fleet_connection.py so fleet followers get the same forwarding (with an inline comment noting per-device-key heterogeneous fleets are a future per-IP-mapping follow-up).
  • @greptile-apps — The previous if aes_128_key is Noneif not aes_128_key truthiness fix landed in fceccd7c0. Updated the now-misleading docstring on test_empty_string_kwarg_* and added a sibling test_empty_string_kwarg_uses_env_when_set covering the YAML/JSON aes_128_key: "" regression you flagged.
  • Added a targeted unit test go2/test_connection.py::test_make_connection_webrtc_forwards_aes_128_key to pin the new make_connection ↔ UnitreeWebRTCConnection contract.

Ran the changes through codex review --commit HEAD after each iteration. Codex caught the missed Go2FleetConnection.start() forwarding (now fixed) and signed off on the amended commit.

@leshy — re. the upstream legion lib transition + lidar perf, leaving that for a separate PR (also flagged on this PR thread by @natcl who noticed degraded nav on Go2 EDU after the upstream switch). Happy to scope that follow-up once you confirm the lidar parser changes have landed in legion upstream.

@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot May 19, 2026
@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot May 19, 2026
@leshy
Copy link
Copy Markdown
Member

leshy commented May 19, 2026

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.

my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect

because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

@mihai-chiorean
Copy link
Copy Markdown
Author

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.

my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect

because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

want me to do that here or is that for a separate PR...? happy to update it, a little worried about the blast radius of updating a package like that since I'm new to the repo.

@mihai-chiorean
Copy link
Copy Markdown
Author

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.
my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect
because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

want me to do that here or is that for a separate PR...? happy to update it, a little worried about the blast radius of updating a package like that since I'm new to the repo.

@leshy?

@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot May 23, 2026
@leshy
Copy link
Copy Markdown
Member

leshy commented May 23, 2026

want me to do that here or is that for a separate PR...? happy to update it, a little worried about the blast radius of updating a package like that since I'm new to the repo.

@leshy?

thx for the ping, I think bump is good, then we will do IRL reviews of the whole feature and can merge right after

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants