Skip to content

Conversation

@apullin
Copy link
Contributor

@apullin apullin commented Apr 28, 2025

  • Adds Accelerometer support for Omi2. Test on hardware.
  • Adds motor support for Omi2. Tested on hardware.
    /claim #1824 for that feature
  • Minor refactoring
  • Changes model & versions in omi.conf and CMake preset

Apologies for all the whitespace changes that are showing up. I am editing in VSCode with default settings (as far as I know),
and it is applying all these minor changes for end-of-line.

This will likely take some discussion. Topics below.

IMU Driver

See the note in accel.c . The only way to actually make it work it to edit the zephyr source tree itself.
Possible TOD here is to see if newer Zephyr distros have a driver for LSM6TR3-C merged.
If so, we might be able to add it into this project, until Nordic SDK moves to a newer Zephyr version with the driver.

Shell commands code

The Omi2 EVT firmware has a number of useful shell commands, which are optionally added here.
But we could/should decide if we want to merge these into the main firmware (optional, behind CONFIG_SHELL), or leave them only in factory FW.

Testing code

In accel.c, there's a function there to test reading from the IMU. Actually, we probably want to delete stuff like this.
TODO: revise.

The big one: DK2 support and source overlap

This update is tested on Omi2 EVT, but it is not tested on DK2.
As of now, we are overlapping with DK2 source.
Project-level, we'll need to decide:

  • Do we have separate impls for DK2 and Omi2 EVT?
  • Common interface but separate impls?
  • Single imp, with everything carefully guarded with macros?

@vercel
Copy link

vercel bot commented Apr 28, 2025

@apullin is attempting to deploy a commit to the kodjima33's projects Team on Vercel.

A member of the Team first needs to authorize it.

@beastoin
Copy link
Collaborator

beastoin commented Apr 28, 2025

Cool man @apullin

1/ Could you reset this code to match the main branch, sir?

Screenshot 2025-04-28 at 15 25 07

2/ did you test on the omi kits ? or any specific board.

--

Regarding your questions:

Do we have separate impls for DK2 and Omi2 EVT?

-> The devkit/devkit2 implementation will be located at: https://github.com/BasedHardware/omi/tree/main/omi/firmware/devkit

-> DK2 and Omi2 EVT in the omi/src/lib are just references to make the porting process easier. They will be removed later.

Common interface but separate impls?
Single imp, with everything carefully guarded with macros?

-> We could also mix these two approaches - building some libs as core so that devkit and omi can use them as shared libs.
-> But let's decide that later. My top priority is to finish the firmware code for the new version first ;)

@apullin
Copy link
Contributor Author

apullin commented Apr 28, 2025

@beastoin

1/ Could you reset this code to match the main branch, sir?

Rebased.

2/ did you test on the omi kits ? or any specific board.

Tested on Omi2 EVT. Motor worked during startup. IMU works when reading, either by including the shell cmd or using the test thread.

Not tested on DK2. But if the entirety of the DK2 project lives in firmware/devkit, the DK2 code would be unaffected by this commit.

@apullin
Copy link
Contributor Author

apullin commented Apr 29, 2025

@beastoin Actually, on the "reset this code" request:

Don't we want differentiation here, as this is an entirely separate hardware revision, separate product, separate firmware?

@beastoin
Copy link
Collaborator

1/ man, i still see your PR made change to the configs which i mentioned. please reset it.

2/ ok ✅

3/ could you resolve the conflict ?

4/ imu is unused for now, any good idea on applying it to our prod ?

@apullin

@apullin apullin force-pushed the omi2_updates branch 3 times, most recently from 813efec to 2afe67a Compare April 30, 2025 15:15
@apullin
Copy link
Contributor Author

apullin commented Apr 30, 2025

@beastoin Conflicts resolved, conf changes reverted.

WRT to use of the IMU - sure, there's plenty of ways to use it. Track motion intensity, read by BLE, add to prompts.
Or shake-to-wake, sleep on no motion, etc.

It looks like reading is already in place in the app. This is all in DK2, so there should be some support for it already? We should coordinate to verify that part still works, so people can build with it.

apullin added 5 commits May 5, 2025 08:39
Includes KConfig option to enable/disable.

Commit 764c45a contains code taken from this commit but merged ahead of it.
Major caveat here: this change cannot work without changes to the zephyr source tree itself.
See comments in accel.c
@beastoin
Copy link
Collaborator

beastoin commented May 9, 2025

1/ ✅ ok
2/ ✅ ok
3/ ✅ ok
4/ ✅ ok, dk2 hasn’t applied imu features yet haha — that’s why i asked. please just set the config to n.
5/ could you please discard the haptic changes? it’s already supported.
6/ why do we need the reset reason in the new firmware version?
https://github.com/BasedHardware/omi/pull/2270/files
7/ also, why do we need this config?
https://github.com/BasedHardware/omi/pull/2270/files#diff-380fa6d740cb65a1fae35ae54bb70caa8f831d2cc6697c26d2391a0ab1b256d1R352
8/ could you please clean the code again? keep the main function clean — just add accel. battery is already supported.

Screenshot 2025-05-09 at 19 06 30 Screenshot 2025-05-09 at 19 07 15 Screenshot 2025-05-09 at 19 08 03

9/ do you plan to support the sdcard in this PR? if not, please discard the sdcard changes.
Screenshot 2025-05-09 at 19 13 32

wip: https://github.com/BasedHardware/omi/tree/main/omi/firmware/omi#wip

overall, appreciate your help man. btw, my suggestion: just keep the accel code with config set to n — and discard all the rest.

@apullin ~

@skywinder
Copy link
Collaborator

@apullin will you update PR?

@skywinder skywinder marked this pull request as draft May 12, 2025 17:10
@apullin
Copy link
Contributor Author

apullin commented May 12, 2025

@apullin will you update PR?

Yes - been busy. Will do within a few days. Also will delete the whitespace changes, now that I've figured out how to do that by rewriting the git commits.

@beastoin
Copy link
Collaborator

yah, feel free to re-popen it when you're ready.

thank man.

@beastoin beastoin closed this May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants