firmware/docs/getting-started_ja.md (1)

Line range hint 33-34: Update the introduction text to match available setup methods

The text mentions "次の3通りの方法" (following 3 methods) but Docker setup is commented out. This should be updated to "次の2通りの方法" (following 2 methods) to match the available options.

-次の3通りの方法があります。
+次の2通りの方法があります。

🧰 Tools

🪛 Markdownlint

20-20: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

21-21: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

22-22: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

firmware/docs/getting-started.md (3)

17-23: Add context about repository structure changes

Since this is a significant change from using submodules to having all code in the main repository, consider adding a brief note explaining this architectural change. Also, showing command output would help users verify successful execution.

 Clone this repository.
+
+> Note: Previously, this project used Git submodules which required the `--recursive` flag.
+> These have now been consolidated into the main repository for simplicity.
 
 ```console
 $ git clone https://github.com/stack-chan/stack-chan.git
 $ cd stack-chan/firmware
 $ npm i
+
+# Expected output:
+added XX packages in Ys

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary>

20-20: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

21-21: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

22-22: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

</details>

</details>

---

Line range hint `39-83`: **Remove commented-out Docker documentation**

Instead of keeping the Docker-related documentation commented out, it should be properly removed. If you want to preserve this information for historical purposes, consider:
1. Moving it to a separate "legacy" or "alternative-setups" document
2. Keeping it in Git history
3. Adding a note about why Docker support was removed

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary>

20-20: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

21-21: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

22-22: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

</details>

</details>

---

Line range hint `1-24`: **Document the consolidated driver modules**

Since this PR moves servo drivers (RS30X and SCServo) into the main repository, consider adding information about:
1. Available driver modules and their purposes
2. How to use these drivers in the firmware
3. Any configuration needed for different servo types

This would help users understand the available options after the removal of external dependencies.

Would you like me to help draft this additional documentation section?

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary>

20-20: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

21-21: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

22-22: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

</details>

</details>

</blockquote></details>
<details>
<summary>firmware/stackchan/drivers/scservo.ts (3)</summary><blockquote>

`22-24`: **Simplify `el` function logic using bitwise operations**

The bitmask `& 0xff00` is unnecessary after shifting `h` by 8 bits. Using bitwise OR improves clarity and efficiency.



Apply this diff to simplify the function:

```diff
22 function el(h: number, l: number) {
23-  return ((h << 8) & 0xff00) + (l & 0xff)
23+  return (h << 8) | (l & 0xff)
24 }

---

19-21: Simplify le function logic by removing redundant bitmask

The bitmask & 0xff00 is redundant when shifting right by 8 bits. Removing it simplifies the code without affecting functionality.

Apply this diff to simplify the function:

19 function le(v: number): [number, number] {
20-  return [(v & 0xff00) >> 8, v & 0xff]
20+  return [v >> 8, v & 0xff]
21 }

---

195-221: Handle promise rejections on timeouts in #sendCommand

Currently, the promise resolves with undefined on a timeout, which might not be expected by the caller. Rejecting the promise on errors provides clearer error handling and aligns with common async/await practices.

Apply this diff to reject the promise on timeout:

213       return new Promise((resolve, reject) => {
214         const id = Timer.set(() => {
215           this.#promises.shift()
216           trace(\`timeout. \${this.#promises.length}\n\`)
217-          resolve(undefined)
217+          reject(new Error('Command timeout'))
218         }, 40)
219         this.#promises.push([resolve, id])
220       })

Ensure that calling methods handle promise rejections appropriately.

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check if the English version has matching changes
# Expected: No --recursive flag in git clone command

rg --type md "git clone.*stack-chan\.git" firmware/docs/getting-started.md

firmware/stackchan/drivers/scservo.ts (4)

58-136: Consider implementing packet validation

The packet handler could benefit from additional validation:

1. Maximum packet size validation
2. Command type validation before processing

 class PacketHandler extends Serial {
+  static readonly MAX_PACKET_SIZE = 64;
   constructor(option) {
     // ...
-    this.#rxBuffer = new Uint8Array(64)
+    this.#rxBuffer = new Uint8Array(PacketHandler.MAX_PACKET_SIZE)
     // ...
   }
   
   // Add validation method
+  private isValidCommand(command: number): command is Command {
+    return Object.values(COMMAND).includes(command as Command);
+  }
 }

---

143-151: Optimize checksum calculation

The current implementation creates a new array slice for each checksum calculation. Consider using array indices directly for better performance.

-function checksum(arr: number[] | Uint8Array): number {
+function checksum(arr: number[] | Uint8Array, length: number): number {
   let sum = 0
-  for (const n of arr.slice(2)) {
+  for (let i = 2; i < length; i++) {
-    sum += n
+    sum += arr[i]
   }
   const cs = ~(sum & 0xff)
   return cs
 }

---

309-310: Extract angle calculation constants

The angle calculations use magic numbers. Consider extracting these into named constants for better maintainability.

+  private static readonly ANGLE_SCALE = 1024;
+  private static readonly ANGLE_RANGE = 200;
+  private static readonly MAX_POSITION = 0x03ff;
+
   async setAngle(angle: number): Promise<unknown> {
-    const a = Math.floor(clamp(((angle + this.#offset) * 1024) / 200, 0, 0x03ff))
+    const a = Math.floor(clamp(
+      ((angle + this.#offset) * SCServo.ANGLE_SCALE) / SCServo.ANGLE_RANGE,
+      0,
+      SCServo.MAX_POSITION
+    ))
     return this.#sendCommand(COMMAND.WRITE, ADDRESS.GOAL_POSITION, ...le(a))
   }

Also applies to: 321-322

---

339-352: Add retry mechanism for readStatus

The readStatus method could benefit from a retry mechanism when receiving corrupted responses, improving reliability.

+  private static readonly MAX_RETRIES = 3;
+  private static readonly RETRY_DELAY = 20;
+
   async readStatus(): Promise<Maybe<{ angle: number }>> {
+    for (let attempt = 0; attempt < SCServo.MAX_RETRIES; attempt++) {
       const values = await this.#sendCommand(COMMAND.READ, ADDRESS.PRESENT_POSITION, 15)
       if (values == null || values.length < 15) {
+        if (attempt < SCServo.MAX_RETRIES - 1) {
+          await new Promise(resolve => Timer.set(resolve, SCServo.RETRY_DELAY));
+          continue;
+        }
         return {
           success: false,
-          reason: 'response corrupted.',
+          reason: `Response corrupted after ${SCServo.MAX_RETRIES} attempts.`,
         }
       }
       const angle = (el(values[0], values[1]) * 200) / 1024
       return {
         success: true,
         value: { angle },
       }
+    }
   }

firmware/stackchan/drivers/rs30x.ts (2)

28-28: Simplify the Rotation type declaration

The Rotation type can be simplified for better readability by directly using keyof typeof Rotation.

Apply this diff:

-export type Rotation = (typeof Rotation)[keyof typeof Rotation]
+export type Rotation = keyof typeof Rotation

---

56-58: Use the 'clamp' utility function for consistency

In the setAngle and setAngleInTime methods, you're clamping the angle values manually. Consider using the existing clamp function for consistency and maintainability.

Apply this diff to use the clamp function:

-const a = Math.max(-150, Math.min(150, angle)) * 10
+const a = clamp(angle, -150, 150) * 10

This change can be applied to both the setAngle and setAngleInTime methods.

Also applies to: 267-267, 278-278

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: meganetaaan
PR: stack-chan/stack-chan#306
File: firmware/stackchan/drivers/scservo.ts:161-161
Timestamp: 2024-11-23T07:45:05.883Z
Learning: Moddableの型定義では、`Timer.set()`関数は`Timer`型を返すため、`Timer`型の変数宣言は正しい。

if (isNaN(angle)) {
  throw new Error('Invalid angle received from servo')
}

#!/bin/bash
# Description: Search for any incorrect references to 'TorqeMode'

# Test: Find all instances of 'TorqeMode' in the codebase.
# Expect: No results if all typos are corrected.
rg 'TorqeMode'

#!/bin/bash
# Let's verify both the correct and incorrect spellings, and also check the definition
# Search for both TorqueMode and TorqeMode
rg -i "torq[ue]*mode"

# Also check the enum/type definition
ast-grep --pattern 'enum TorqueMode {
  $$$
}'

#!/bin/bash
# Let's check the complete definition and usage context in rs30x.ts
rg -A 3 "type TorqueMode|export const TorqueMode" firmware/stackchan/drivers/rs30x.ts