Added beepType options to cancel and configure alerts command #429
Conversation
|
I've updated this PR to the latest Omnikit branch to review the sound options for cancelDeliveryType. It works properly using the Tests commands on a live Pod. I still have some error in Travis to look at, but almost there. The way I setup the sounds does differ currently with the way it is called on the 0x19 command using It is probably better to make one beep method which can be reused in all commands, also to be able to make a setup page to let users choose sound setups for different things. What do you think? |
ps2
left a comment
ps2
left a comment
There was a problem hiding this comment.
Thanks! Need to get tests passing too.
| public enum CancelType { | ||
| case normal | ||
| case suspend | ||
| public struct BeepType: OptionSet { |
There was a problem hiding this comment.
BeepType should be an enum, not an OptionSet
Seperated Bolus tests from Message tests.
|
Let me know if this is ok. Then I will look at combining the same enum beep setup on the 0x19 command. |
Lytrix
changed the title
| } | ||
|
|
||
| self.beepType = (UInt16(encodedData[4]) << 8) + UInt16(encodedData[5]) | ||
| self.beepType = BeepType(rawValue: encodedData[4])! |
There was a problem hiding this comment.
This should throw an exception if the rawValue isn't valid
There was a problem hiding this comment.
Do you have an example how I can assert this? Would be good to write a test to see if it fails on values higher than 8.
There was a problem hiding this comment.
guard let beepType = BeepType(rawValue: encodedData[4]) else {
throw MessageError.unknownValue(value: encodedData[4], typeDescription: "BeepType")
}
self.beepType = beepType
| } | ||
| data.appendBigEndian(beepType) | ||
| data.appendBigEndian(UInt16(beepType.rawValue) << 8 + UInt16(beepRepeat)) |
There was a problem hiding this comment.
If these are two bytes, you could just append each one in turn, rather than encoding them into a UInt16, and then splitting them back up into something to be appended to the data object.
There was a problem hiding this comment.
Good idea, this is a bit overkill.
Lytrix
changed the title
| data.appendBigEndian(minutes) | ||
| } | ||
| data.appendBigEndian(UInt16(beepType.rawValue) << 8 + UInt16(beepRepeat)) | ||
| data.append(UInt8(beepType.rawValue)) |
| } | ||
| data.appendBigEndian(UInt16(beepType.rawValue) << 8 + UInt16(beepRepeat)) | ||
| data.append(UInt8(beepType.rawValue)) | ||
| data.append(UInt8(beepRepeat)) |
There was a problem hiding this comment.
If you change beepRepeat to a UInt8, then you don't need to worry about casting, and possible errors caused by that (if you cast a number > 255 into a UInt8 you'll get an exception)
There was a problem hiding this comment.
Yes, I was unsure where it is essential to explicitly declare the cast type.
| } | ||
| let beepTypeBits = encodedData[4] | ||
| guard let beepType = BeepType(rawValue: beepTypeBits) else { | ||
| throw MessageError.unknownValue(value: beepTypeBits, typeDescription: "beepType") |
There was a problem hiding this comment.
Small suggestion: typeDescription is intended to have name of the enum type that we're missing a value for, so "beepType" should be capitalized.
|
Thanks @Lytrix ! |
2 participants
This looks like a lot better PR. ;-)
I've updated cancelDelivery.
Added tests with cancel temp basal and bolus.
Fixed the missing values cancelDelivery variables in the podSessions.
The suspend command still needs to be added into this with this test: 1f 05 b15898b0
03which I also have ready, but I wanted to make it testable with a 0x19 command as well.