vibration motor control refactor (low level only)#332
vibration motor control refactor (low level only)#332424778940z merged 1 commit intoOneKeyHQ:mainfrom
Conversation
WalkthroughThe motor module was refactored. Old methods for vibration were removed. New methods for specific vibration patterns were added. The motor control logic was unified and now supports both CPU and timer modes. Several new vibration sequences were defined. The Python interface was updated to use the new API. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/embed/trezorhal/motor_patterns.h (2)
8-38: Typo:durnation_us
Spelling.
40-42: Inconsistent nameMAL_relax
Case differs.core/embed/trezorhal/motor.h (2)
20-23: Typo in field name
durnation_usis misspelled.
31-31: Missing void in prototype
motor_reset()lacksvoidin its parameter list.core/embed/trezorhal/motor.c (1)
138-138: Shadowed identifierParameter
cpu_playhides the global with the same name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (7)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/firmware/main.c(1 hunks)core/embed/trezorhal/device.c(3 hunks)core/embed/trezorhal/motor.c(1 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/embed/trezorhal/util_macros.h(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- core/embed/firmware/main.c
- core/embed/trezorhal/device.c
- core/embed/trezorhal/util_macros.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
🧰 Additional context used
🧬 Code Graph Analysis (3)
core/embed/firmware/main.c (2)
core/embed/trezorhal/motor.h (1)
motor_init(26-26)core/embed/trezorhal/motor.c (1)
motor_init(110-116)
core/embed/trezorhal/device.c (2)
core/embed/trezorhal/motor.h (2)
motor_init(26-26)motor_play_medium(39-39)core/embed/trezorhal/motor.c (2)
motor_init(110-116)motor_play_medium(212-215)
core/embed/trezorhal/motor.c (2)
core/embed/trezorhal/motor.h (15)
motor_ctrl(28-28)motor_reset(31-31)motor_init(26-26)motor_deinit(27-27)motor_is_busy(29-29)motor_play(30-30)motor_resonant_finder(34-34)motor_play_whisper(37-37)motor_play_light(38-38)motor_play_medium(39-39)motor_play_heavy(40-40)motor_play_success(43-43)motor_play_warning(44-44)motor_play_error(45-45)motor_play_slide(46-46)core/embed/trezorhal/motor_patterns.h (8)
seq_Success(45-69)seq_Success(45-45)seq_Warning(71-95)seq_Warning(71-71)seq_Error(97-134)seq_Error(97-97)seq_Slide(136-167)seq_Slide(136-136)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (1)
core/embed/trezorhal/motor_patterns.h (1)
45-69: Risk of overflow inseq_Success
No size check.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
core/embed/trezorhal/motor.h (1)
4-8: Missing stddef.h include
♻️ Duplicate comments (2)
core/embed/trezorhal/motor.c (2)
89-96: Potential out-of-bounds on last element
116-121: PinState may be 2, not 0/1
🧹 Nitpick comments (2)
core/embed/trezorhal/motor.c (1)
125-135:cpu_playshadowed, hurts claritycore/src/trezor/motor.py (1)
11-17: Label mismatch
Prints say “weak/strong” but code plays “medium/heavy”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (7)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/firmware/main.c(1 hunks)core/embed/trezorhal/device.c(3 hunks)core/embed/trezorhal/motor.c(3 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/src/trezor/motor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/embed/trezorhal/device.c
- core/embed/firmware/main.c
- core/embed/trezorhal/motor_patterns.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/embed/trezorhal/motor.c (2)
core/embed/trezorhal/motor.h (15)
motor_ctrl(28-28)motor_reset(31-31)motor_init(26-26)motor_deinit(27-27)motor_is_busy(29-29)motor_play(30-30)motor_resonant_finder(34-34)motor_play_whisper(37-37)motor_play_light(38-38)motor_play_medium(39-39)motor_play_heavy(40-40)motor_play_success(43-43)motor_play_warning(44-44)motor_play_error(45-45)motor_play_slide(46-46)core/embed/trezorhal/motor_patterns.h (8)
seq_Success(45-68)seq_Success(45-45)seq_Warning(70-93)seq_Warning(70-70)seq_Error(95-131)seq_Error(95-95)seq_Slide(133-163)seq_Slide(133-133)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Defs check
- GitHub Check: Gen check
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
core/embed/trezorhal/motor.h (1)
4-5: Missing<stddef.h>include
Size t not guaranteed.
♻️ Duplicate comments (1)
core/embed/trezorhal/motor.c (1)
118-119: InvalidPinStatevalue still present
Bit mask can return 2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (6)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/trezorhal/device.c(1 hunks)core/embed/trezorhal/motor.c(3 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/src/trezor/motor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/embed/trezorhal/device.c
- core/src/trezor/motor.py
- core/embed/trezorhal/motor_patterns.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Style check
- GitHub Check: Defs check
424778940z
force-pushed
the
main
branch
3 times, most recently
from
592916e to
1f76e69
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
core/embed/trezorhal/motor.h (1)
1-9: Missing extern "C" guard
♻️ Duplicate comments (1)
core/embed/trezorhal/motor.c (1)
118-119: Invalid PinState value
Still passes 2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (6)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/trezorhal/device.c(1 hunks)core/embed/trezorhal/motor.c(3 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/src/trezor/motor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/embed/trezorhal/device.c
- core/src/trezor/motor.py
- core/embed/trezorhal/motor_patterns.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Style check
- GitHub Check: Defs check
🔇 Additional comments (2)
core/embed/trezorhal/motor.c (1)
205-228: Possible buffer overflow
Sequence length unchecked.core/embed/trezorhal/motor.h (1)
32-32:motor_resetunclear after refactor
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/embed/trezorhal/motor.c (2)
19-26: Volatile missing for ISR variables.Variables accessed in interrupt handler need volatile qualifier.
118-119: GPIO pin state values incorrect.Bitwise AND produces 0 or 2, but HAL_GPIO_WritePin expects 0 or 1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (6)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/trezorhal/device.c(1 hunks)core/embed/trezorhal/motor.c(3 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/src/trezor/motor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/embed/trezorhal/device.c
- core/src/trezor/motor.py
- core/embed/trezorhal/motor_patterns.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
- core/embed/trezorhal/motor.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Defs check
- GitHub Check: Gen check
- GitHub Check: Style check
🔇 Additional comments (2)
core/embed/trezorhal/motor.c (2)
89-94: Bounds check looks correct now.The out-of-bounds access risk appears to be properly addressed.
124-157: Motor play function well-structured.Good separation of CPU vs timer modes with proper bounds checking.
Signed-off-by: Adam BZH <adam@onekey.so>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/embed/trezorhal/motor.h (1)
21-24: Packed struct still has alignment issues.The packed attribute remains despite previous review feedback about potential misalignment problems.
core/embed/trezorhal/motor.c (2)
19-26: Variables accessed in ISR need volatile qualifier.These variables are used in TIM7_IRQHandler but aren't marked volatile.
206-228: Buffer overflow risk in sequence functions.The act_list_combine[128] buffer lacks bounds checking when sequence functions write to it.
🧹 Nitpick comments (1)
core/embed/trezorhal/motor.c (1)
11-16: Pragma warnings hide potential issues.Suppressing unused-function and unused-variable warnings may mask real problems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (6)
core/embed/extmod/modtrezorio/modtrezorio-motor.h(1 hunks)core/embed/trezorhal/device.c(1 hunks)core/embed/trezorhal/motor.c(3 hunks)core/embed/trezorhal/motor.h(2 hunks)core/embed/trezorhal/motor_patterns.h(1 hunks)core/src/trezor/motor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/trezor/motor.py
- core/embed/trezorhal/device.c
- core/embed/trezorhal/motor_patterns.h
- core/embed/extmod/modtrezorio/modtrezorio-motor.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/embed/trezorhal/motor.c (2)
core/embed/trezorhal/motor.h (16)
motor_ctrl(29-29)motor_reset(32-32)motor_init(27-27)motor_deinit(28-28)motor_is_busy(30-30)motor_play(31-31)motor_resonant_finder(35-35)motor_set_builtin_play_method(38-38)motor_play_whisper(41-41)motor_play_light(42-42)motor_play_medium(43-43)motor_play_heavy(44-44)motor_play_success(47-47)motor_play_warning(48-48)motor_play_error(49-49)motor_play_slide(50-50)core/embed/trezorhal/motor_patterns.h (8)
seq_Success(45-68)seq_Success(45-45)seq_Warning(70-93)seq_Warning(70-70)seq_Error(95-131)seq_Error(95-95)seq_Slide(133-163)seq_Slide(133-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Defs check
- GitHub Check: Style check
- GitHub Check: Gen check
🔇 Additional comments (4)
core/embed/trezorhal/motor.h (2)
5-5: Good addition for size_t support.The stddef.h include is needed for the size_t parameter in motor_play function.
27-32: New API consolidates motor control.The unified motor_play function with CPU/timer mode selection is a good design improvement.
core/embed/trezorhal/motor.c (2)
115-120: Bitwise operations produce correct GPIO states.The motor control logic correctly uses bitwise AND operations for GPIO pin states.
124-157: Unified motor_play function improves API design.The combined CPU/timer mode approach simplifies the interface while maintaining flexibility.
Signed-off-by: Adam BZH <adam@onekey.so>
Signed-off-by: Adam BZH <adam@onekey.so>
4 participants
Major refactor for motor control
All low level api are adapted, MP layer change still needed
Timing may still needs adjust after MP layer adapted
Summary by CodeRabbit
New Features
Refactor