Skip to content

Added calibration checking to gyro#171

Open
T1mothyyang wants to merge 7 commits intomasterfrom
gyro-calibration
Open

Added calibration checking to gyro#171
T1mothyyang wants to merge 7 commits intomasterfrom
gyro-calibration

Conversation

@T1mothyyang
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Feb 11, 2018

Codecov Report

Merging #171 into master will decrease coverage by 0.59%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #171     +/-   ##
=========================================
- Coverage   72.23%   71.63%   -0.6%     
=========================================
  Files          79       79             
  Lines        1563     1576     +13     
  Branches      139      136      -3     
=========================================
  Hits         1129     1129             
- Misses        434      447     +13
Impacted Files Coverage Δ
...okrobotics/potassium/sensors/imu/DigitalGyro.scala 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b3b4e1...1eccf8e. Read the comment docs.

Copy link
Contributor

@PhilipAxelrod PhilipAxelrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good

val part4 = part3._2.splitAt(calibrationVelocities.size / 5)
List(part1._1.toList, part2._1.toList, part3._1.toList, part4._1.toList, part4._2.toList)
}
println("Checking Calibration")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove print which may cause preformance degredation

acc + cur
}
val average = sumChunk * (1D / chunk.size)
println(s"Chunk Average: $average")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove print

val average = sumChunk * (1D / chunk.size)
println(s"Chunk Average: $average")
if((average.x - currentDrift.x).abs > threshold.x){
println(s"Drift is not within x threshold")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove print

List(part1._1.toList, part2._1.toList, part3._1.toList, part4._1.toList, part4._2.toList)
}
println("Checking Calibration")
chunks.foreach({chunk: List[Value3D[AngularVelocity]] =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use forAll instead

successful = false
}
})
successful
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check that the queue of calibration values is full

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On that note, please make the queue size 1,000 values instead if 200. This means that we'll use 5 seconds worth of data instead of 1 second (at 5ms update loops)

}
}

def checkCalibration: Boolean = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more like, checking if calibration is in valid. Please name accordingly

* @param tickPeriod tick period of robot
*/
abstract class DigitalGyro(tickPeriod: Time) {
abstract class DigitalGyro(tickPeriod: Time, threshold: Value3D[AngularVelocity]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to describe its use in checking for invalid calibration

*/
abstract class DigitalGyro(tickPeriod: Time) {
abstract class DigitalGyro(tickPeriod: Time, threshold: Value3D[AngularVelocity]) {
// Tick Period of the robot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment shouldn't be here?

def checkCalibration: Boolean = {
var successful = true
val chunks: List[List[Value3D[AngularVelocity]]] = {
val part1 = calibrationVelocities.splitAt(calibrationVelocities.size / 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😀 yay for Scala collection APIs!

@shadaj
Copy link
Contributor

shadaj commented Feb 15, 2018

Ping @Wafflemans.

@PhilipAxelrod
Copy link
Contributor

Ping @Wafflemans please make these fixes

Copy link
Contributor

@PhilipAxelrod PhilipAxelrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (compiles) {
"lgtm"
} else {
"not lgtm"
}

Copy link
Contributor

@shadaj shadaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass

* An interface for communicating with the ADIS16448 IMU.
*/
class ADIS16448(spi: SPITrait, updatePeriod: Time) extends DigitalGyro(updatePeriod) {
class ADIS16448(spi: SPITrait, updatePeriod: Time, maxDriftDeviation: Value3D[AngularVelocity]) extends DigitalGyro(updatePeriod, maxDriftDeviation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making this a constructor parameter here and in DigitalGyro, can you make it a parameter to the check method?

Copy link
Contributor

@PhilipAxelrod PhilipAxelrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

val part2 = part1._2.splitAt(calibrationVelocities.size / 5)
val part3 = part2._2.splitAt(calibrationVelocities.size / 5)
val part4 = part3._2.splitAt(calibrationVelocities.size / 5)
Seq(part1._1.toList, part2._1.toList, part3._1.toList, part4._1.toList, part4._2.toList)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These toLists are a performance burden since they forcibly convert the sequences into linked lists.

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