Skip to content

Commit

Permalink
Merge pull request #64 from NordicSemiconductor/out-of-order-write-re…
Browse files Browse the repository at this point in the history 
…sponse

1.2.4 Version
  • Loading branch information
@dinesharjani
dinesharjani authored Jul 4, 2022
2 parents cd25783 + 1afb37e commit 6426e06
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 55 deletions.
4 changes: 2 additions & 2 deletions Example/nRF Connect Device Manager.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@
"$(inherited)",
"@executable_path/Frameworks",
);
MARKETING_VERSION = 1.2.3;
MARKETING_VERSION = 1.2.4;
PRODUCT_BUNDLE_IDENTIFIER = com.nordicsemi.nrfconnect.devicemanager;
PRODUCT_NAME = "Device Manager";
SWIFT_VERSION = 5.0;
Expand All @@ -563,7 +563,7 @@
"$(inherited)",
"@executable_path/Frameworks",
);
MARKETING_VERSION = 1.2.3;
MARKETING_VERSION = 1.2.4;
PRODUCT_BUNDLE_IDENTIFIER = com.nordicsemi.nrfconnect.devicemanager;
PRODUCT_NAME = "Device Manager";
SWIFT_VERSION = 5.0;
Expand Down
2 changes: 1 addition & 1 deletion Source/Bluetooth/McuMgrBleTransport.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ extension McuMgrBleTransport: McuMgrTransport {
switch self._send(data: data, timeoutInSeconds: timeout) {
case .failure(McuMgrTransportError.waitAndRetry):
sleep(UInt32(McuMgrBleTransportConstant.WAIT_AND_RETRY_INTERVAL))
self.log(msg: "Retry \(i)", atLevel: .info)
self.log(msg: "Retry \(i + 1)", atLevel: .info)
case .failure(let error):
self.log(msg: error.localizedDescription, atLevel: .error)
DispatchQueue.main.async {
Expand Down
5 changes: 5 additions & 0 deletions Source/Managers/DFU/FirmwareUpgradeManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,11 @@ public struct FirmwareUpgradeConfiguration: Codable {
/// Can be used in conjunction with SMP Pipelining.
public var reassemblyBufferSize: UInt64

/// SMP Pipelining is considered Enabled for `pipelineDepth` values larger than `1`.
public var pipeliningEnabled: Bool {
return pipelineDepth > 1
}

public init(eraseAppSettings: Bool = true, pipelineDepth: Int = 1, byteAlignment: ImageUploadAlignment = .disabled,
reassemblyBufferSize: UInt64 = 0) {
self.eraseAppSettings = eraseAppSettings
Expand Down
72 changes: 46 additions & 26 deletions Source/Managers/ImageManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public class ImageManager: McuManager {

override class var TAG: McuMgrLogCategory { .image }

private static let PIPELINED_WRITES_TIMEOUT_SECONDS = 10
private static let truncatedHashLen = 3

// MARK: - IDs
Expand Down Expand Up @@ -73,7 +74,16 @@ public class ImageManager: McuManager {
payload.updateValue(CBOR.byteString([UInt8](data.sha256()[0..<ImageManager.truncatedHashLen])), forKey: "sha")
}

send(op: .write, commandId: ImageID.Upload, payload: payload, callback: callback)
guard uploadConfiguration.pipeliningEnabled else {
send(op: .write, sequenceNumber: 0, commandId: ImageID.Upload, payload: payload, callback: callback)
return
}

send(op: .write, sequenceNumber: uploadSequenceNumber, commandId: ImageID.Upload, payload: payload,
callback: callback)

uploadExpectedOffsets.append((uploadSequenceNumber, chunkEnd))
uploadSequenceNumber = uploadSequenceNumber == .max ? 0 : uploadSequenceNumber + 1
}

/// Test the image with the provided hash.
Expand Down Expand Up @@ -155,15 +165,14 @@ public class ImageManager: McuManager {
uploadIndex = 0
uploadExpectedOffsets = []
uploadLastOffset = 0
uploadSequenceNumber = 0
uploadConfiguration = configuration
if let bleTransport = transporter as? McuMgrBleTransport {
bleTransport.numberOfParallelWrites = configuration.pipelineDepth
bleTransport.chunkSendDataToMtuSize = configuration.reassemblyBufferSize != 0
}

log(msg: "Uploading image \(firstImage.image) (\(firstImage.data.count) bytes)...", atLevel: .verbose)
let firstOffset = maxDataPacketLengthFor(data: firstImage.data, image: firstImage.image, offset: 0)
uploadExpectedOffsets.append(firstOffset)
upload(data: firstImage.data, image: firstImage.image, offset: 0,
alignment: configuration.byteAlignment,
callback: uploadCallback)
Expand Down Expand Up @@ -233,8 +242,11 @@ public class ImageManager: McuManager {
private var uploadIndex: Int = 0
/// Current image byte offset to send from.
private var uploadLastOffset: UInt64!
/// Each upload packet gets its own Sequence Number, which we rotate
/// within the bounds of an unsigned UInt8 [0...255].
private var uploadSequenceNumber: UInt8 = 0

private var uploadExpectedOffsets: [UInt64] = []
private var uploadExpectedOffsets: [(sequenceNumber: UInt8, offset: UInt64)] = []
/// The sequence of images we want to send to the device.
private var uploadImages: [Image]?
/// Delegate to send image upload updates to.
Expand Down Expand Up @@ -314,10 +326,8 @@ public class ImageManager: McuManager {
uploadState = .uploading
let offset = uploadLastOffset ?? 0
log(msg: "Resuming uploading image \(image) from \(offset)/\(imageData.count)...", atLevel: .application)
let firstResumeOffset = offset + maxDataPacketLengthFor(data: imageData, image: image, offset: offset)
uploadExpectedOffsets.append(firstResumeOffset)
upload(data: imageData, image: image, offset: offset, alignment: uploadConfiguration.byteAlignment,
callback: uploadCallback)
callback: uploadCallback)
} else {
log(msg: "Upload has not been previously paused", atLevel: .warning)
}
Expand All @@ -333,6 +343,10 @@ public class ImageManager: McuManager {
return
}

if #available(iOS 10.0, *) {
dispatchPrecondition(condition: .onQueue(.main))
}

// Check for an error.
if let error = error {
if case let McuMgrTransportError.insufficientMtu(newMtu) = error {
Expand Down Expand Up @@ -363,20 +377,21 @@ public class ImageManager: McuManager {
}

if let offset = response.off {
// Pipelining requires the use of byte-alignment, and byte-alignment is required
// because otherwise we can't predict how many bytes the firmware will accept.
if self.uploadConfiguration.pipelineDepth > 1 {
if let uploadIndex = self.uploadExpectedOffsets.firstIndex(of: UInt64(offset)) {
self.uploadExpectedOffsets.remove(at: uploadIndex)
} else {
// We've missed an offset, so let's compensate or else the upload will stall.
self.log(msg: "Missed ACK for offset \(offset), image \(self.uploadIndex). Clearing first offset (\(self.uploadExpectedOffsets.first ?? 0)) to compensate.", atLevel: .warning)
self.uploadExpectedOffsets.removeFirst()
var packetReceivedOutOfOrder = false

// Note that pipelining requires the use of byte-alignment, otherwise we
// can't predict how many bytes the firmware will accept in each chunk.
if self.uploadConfiguration.pipeliningEnabled {
guard let i = self.uploadExpectedOffsets.firstIndex(where: { $0.sequenceNumber == response.header.sequenceNumber }) else {
self.cancelUpload(error: ImageUploadError.invalidUploadSequenceNumber(response.header.sequenceNumber))
return
}
} else {
// So if we're not pipelining, we usually don't apply byte alignment.
// And even if we did, we don't need to 'predict' offsets, so we don't care.
self.uploadExpectedOffsets.removeAll()

packetReceivedOutOfOrder = i != 0
if packetReceivedOutOfOrder {
self.log(msg: "OOD Packet: Received Seq No. \(response.header.sequenceNumber) instead of expected Seq No. \(self.uploadExpectedOffsets[0].sequenceNumber)", atLevel: .debug)
}
self.uploadExpectedOffsets.remove(at: i)
}

self.uploadLastOffset = max(self.uploadLastOffset, UInt64(offset))
Expand Down Expand Up @@ -413,23 +428,24 @@ public class ImageManager: McuManager {
// Don't trigger writes to another image unless all write(s) have returned for
// the current one.
guard self.uploadExpectedOffsets.isEmpty else { return }
let firstPacketOffset = self.maxDataPacketLengthFor(data: images[self.uploadIndex].data, image: self.uploadIndex, offset: 0)
self.uploadExpectedOffsets.append(firstPacketOffset)
self.uploadLastOffset = 0
self.sendNext(from: UInt64(0))
}
return
}

guard !packetReceivedOutOfOrder || self.uploadExpectedOffsets.isEmpty else {
// If packet was received OOD, we must throttle to allow device to catch-up.
// If there's no pipelining, `uploadExpectedOffsets` will always be empty here.
return
}

for i in 0..<(self.uploadConfiguration.pipelineDepth - self.uploadExpectedOffsets.count) {
guard let chunkOffset = self.uploadExpectedOffsets.last ?? self.uploadLastOffset,
guard let chunkOffset = self.uploadExpectedOffsets.last?.offset ?? self.uploadLastOffset,
chunkOffset < self.imageData?.count ?? 0 else {
// No remaining chunks to be sent.
return
}

let chunkSize = self.maxDataPacketLengthFor(data: images[self.uploadIndex].data, image: self.uploadIndex, offset: chunkOffset)
self.uploadExpectedOffsets.append(chunkOffset + chunkSize)
self.sendNext(from: chunkOffset)
}
} else {
Expand Down Expand Up @@ -538,6 +554,8 @@ public enum ImageUploadError: Error {
case invalidPayload
/// Image Data is nil.
case invalidData

case invalidUploadSequenceNumber(UInt8)
/// McuMgrResponse contains a error return code.
case mcuMgrErrorCode(McuMgrReturnCode)
}
Expand All @@ -550,6 +568,8 @@ extension ImageUploadError: LocalizedError {
return "Response payload values do not exist."
case .invalidData:
return "Image data is nil."
case .invalidUploadSequenceNumber(let sequenceNumber):
return "Received Response for Unknown Sequence Number \(sequenceNumber)."
case .mcuMgrErrorCode(let code):
return "Remote error: \(code)."
}
Expand Down
34 changes: 9 additions & 25 deletions Source/McuManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ open class McuManager {

/// Logger delegate will receive logs.
public weak var logDelegate: McuMgrLogDelegate?
/// Each Packet gets its own Sequence Number, which we will rotate for every
/// packet sent within the bounds of an unsigned UInt8, so 0...255 (inclusive).
private var sequenceNumber: UInt8

//**************************************************************************
// MARK: Initializers
Expand All @@ -56,39 +53,36 @@ open class McuManager {
public init(group: McuMgrGroup, transporter: McuMgrTransport) {
self.group = group
self.transporter = transporter
self.sequenceNumber = 0
self.mtu = McuManager.getDefaultMtu(scheme: transporter.getScheme())
}

// MARK: - Send

public func send<T: McuMgrResponse, R: RawRepresentable>(op: McuMgrOperation,
public func send<T: McuMgrResponse, R: RawRepresentable>(op: McuMgrOperation, sequenceNumber: UInt8 = 0,
commandId: R, payload: [String:CBOR]?,
timeout: Int = DEFAULT_SEND_TIMEOUT_SECONDS,
callback: @escaping McuMgrCallback<T>) where R.RawValue == UInt8 {
send(op: op, flags: 0, commandId: commandId, payload: payload, callback: callback)
send(op: op, sequenceNumber: sequenceNumber, flags: 0, commandId: commandId, payload: payload, timeout: timeout,
callback: callback)
}

public func send<T: McuMgrResponse, R: RawRepresentable>(op: McuMgrOperation, flags: UInt8,
commandId: R, payload: [String:CBOR]?,
public func send<T: McuMgrResponse, R: RawRepresentable>(op: McuMgrOperation, sequenceNumber: UInt8,
flags: UInt8, commandId: R, payload: [String:CBOR]?,
timeout: Int = DEFAULT_SEND_TIMEOUT_SECONDS,
callback: @escaping McuMgrCallback<T>) where R.RawValue == UInt8 {
let packetPacketSequenceNumber = sequenceNumber
sequenceNumber = sequenceNumber.next()

log(msg: "Sending \(op) command (Group: \(group), seq: \(packetPacketSequenceNumber), ID: \(commandId)): \(payload?.debugDescription ?? "nil")",
log(msg: "Sending \(op) command (Group: \(group), seq: \(sequenceNumber), ID: \(commandId)): \(payload?.debugDescription ?? "nil")",
atLevel: .verbose)
let mcuPacketData = McuManager.buildPacket(scheme: transporter.getScheme(), op: op,
flags: flags, group: group.uInt16Value,
sequenceNumber: packetPacketSequenceNumber,
sequenceNumber: sequenceNumber,
commandId: commandId, payload: payload)
let _callback: McuMgrCallback<T> = logDelegate == nil ? callback : { [weak self] (response, error) in
if let self = self {
if let response = response {
self.log(msg: "Response (Group: \(self.group), seq: \(packetPacketSequenceNumber), ID: \(response.header!.commandId!)): \(response)",
self.log(msg: "Response (Group: \(self.group), seq: \(sequenceNumber), ID: \(response.header!.commandId!)): \(response)",
atLevel: .verbose)
} else if let error = error {
self.log(msg: "Request (Group: \(self.group), seq: \(packetPacketSequenceNumber)) failed: \(error.localizedDescription))",
self.log(msg: "Request (Group: \(self.group), seq: \(sequenceNumber)) failed: \(error.localizedDescription))",
atLevel: .error)
}
}
Expand Down Expand Up @@ -336,13 +330,3 @@ extension McuMgrReturnCode: CustomStringConvertible {
}
}
}

// MARK: - UInt8

extension UInt8 {

func next() -> UInt8 {
guard self != .max else { return 0}
return self + 1
}
}
2 changes: 1 addition & 1 deletion iOSMcuManagerLibrary.podspec
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = "iOSMcuManagerLibrary"
s.version = "1.2.3"
s.version = "1.2.4"
s.license = { :type => "Apache 2.0", :file => "LICENSE" }
s.summary = "A mobile management library for devices running Apache Mynewt or Zephyr."
s.homepage = "https://github.com/NordicSemiconductor/IOS-nRF-Connect-Device-Manager"
Expand Down

0 comments on commit 6426e06

Please sign in to comment.