Draft for Discussion: mlawlerau #36

mlawlerau · 2021-02-22T19:10:07Z

Hi Mickey, my first GitHub PR so i might be missing or confused about somethings so help me out if I've got something wrong. Here is a draft PR of all my current changes. I've documented them below. Not expecting to complete this PR, just wanted you to see what I have been doing. Happy to discuss why i changed things and perhaps you might suggest better ways of doing things. I am new at objective-c and swift, coming from a java/groovy background.

LTODB2Adapter

Made enums Swift friendly

LTOBD2Command

Added 'LTResponseDataType' enum
Extended concepts of Command to support accessing the response value in a typed manner independently of a formatted string.
- i.e. for a temperature, you can access the reponse as an integer or a double value that can be used in an application directly
- this requires inserting a new conceptual layer 'decodeResponse' which can turn the unpacked array of bytes into a typed object, but not convert to string or apply formatting/units
- formatted string variant of response value still available - formattedResponse() simply calls decodeResponse() to do the decoding first, before it applies formatting and units.
- units and format strings generalised as configurable properties for abstract/concrete Command classes, enabling sensible defaults and overrides where necessary.
Introduced new method 'decodeResponse'
Renamed 'cookedResponse' to 'responsePayload'
Renamed 'didCookResponse' to 'didUnpackResponsePayload'

LTOBD2PID

Added 'LTOBD2PIDInteger' and 'LTOBD2PIDDouble' abstract subclassses for PIDs whose response payload decodes to single Integer and Double objects.
Enhanced concepts of a PID command to support
Updated various PID commands to extend new abstract subclassses.
More PID commands for query supported PID commands in additional hex ranges
Odometer command (untested, not supported by my car)

LTVIN

Added support for Model Year Range Discrimination (alas doesnt work for Porsche 997.2, plan to fix with pragmatic workaround)

LTBTLESerialTransporter

Added 'adapter' property to expose CBPeripheral
Temporary workaround to ignore Bluetooth BatteryMonitor
- My car's Bluetooth Battery Monitor advertises with the same characteristcs as my ELM327 adapter.
- Plan to extend functionality to support a known set of user-configurable identifiers to be excluded from connection

Localizable.strings

Minor typos
Add strings for Model Year decoding

- Made enums Swift friendly LTOBD2Command - Added 'LTResponseDataType' enum - Extended concepts of Command to support accessing the response value in a typed manner independently of a formatted string. - i.e. for a temperature, you can access the reponse as an integer or a double value that can be used in an application directly - this requires inserting a new conceptual layer 'decodeResponse' which can turn the unpacked array of bytes into a typed object, but not convert to string or apply formatting/units - formatted string variant of response value still available - formattedResponse() simply calls decodeResponse() to do the decoding first, before it applies formatting and unit. - units and format strings generalised as configurable properties for abstract/concrete Command classes, enabling sensible defaults and overrides where necessary. - Introduced new method 'decodeResponse' - Renamed 'cookedResponse' to 'responsePayload' - Renamed 'didCookResponse' to 'didUnpackResponsePayload' LTOBD2PID - Added 'LTOBD2PIDInteger' and 'LTOBD2PIDDouble' abstract subclassses for PIDs whose response payload decodes to single Integer and Double objects. - Enhanced concepts of a PID command to support - Updated various PID commands to extend new abstract subclassses. - More PID commands for query supported PID commands in additional hex ranges - Odometer command (untested, not supported by my car) LTVIN - Added support for Model Year Range Discrimination (alas doesnt work for Porsche 997.2, plan to fix with pragmatic workaround) LTBTLESerialTransporter - Added 'adapter' property to expose CBPeripheral - Temporary workaround to ignore Bluetooth BatteryMonitor - My car's Bluetooth Battery Monitor advertises with the same characteristcs as my ELM327 adapter. - Plan to extend functionality to support a known set of user-configurable identifiers to be excluded from connection Localizable.strings - Minor typos - Add strings for Model Year decoding

mickeyl · 2021-03-26T08:53:23Z

First off, sorry for the delay in answering. I'm super busy with a new project (more on that later, but it has to do with car diagnostics).

Thanks a lot for your pull request. I will start commenting on the individual parts of the pull request in detail right after finishing this message. In general, what we should aim for is more fine granular commits, so that we have a chance to retrace the history.

Please see the inline comments. Looking forward to merging this contribution!

mickeyl · 2021-03-26T08:57:13Z

LTSupportAutomotive/LTBTLESerialTransporter.m

+
+    // Temp workaround to ignore pesky Bluetooth Battery Monitor
+    if ( [peripheral.name isEqualToString:@"Battery Monitor"] )
+    {
+        LOG( @"Peripheral '%@' is that darn Battery Monitor", peripheral.identifier );
+
+        [_manager cancelPeripheralConnection:peripheral];
+        [_possibleAdapters removeObject:peripheral];
+        return;
+    }


This seems a bit unnecessary. I noticed you wrote "temp workaround", but still… usually you are querying for dedicated services – is that battery monitor of yours providing the same service UUID as one of your OBD2 adapters?

Agree. I have removed this from my code now. This won't be in my next PR.

mickeyl · 2021-03-26T08:58:00Z

LTSupportAutomotive/LTOBD2Adapter.m

@@ -61,7 +61,7 @@ -(void)didCompleteResponse:(NSArray<NSString*>*)lines protocol:(LTOBD2Protocol*)
    {
        if ( ! [lines.firstObject isEqualToString:RESPONSE_FINAL_NODATA] )
        {
-            [_command didCookResponse:[protocol decode:lines originatingCommand:_command.commandString] withProtocolType:protocolType];
+            [_command didUnpackResponsePayload:[protocol decode:lines originatingCommand:_command.commandString] withProtocolType:protocolType];


Is this merely a cosmetic change or what is your reason renaming that?

Ok, after reading more of it I get the semantic difference. Fine with me.

mickeyl · 2021-03-26T09:01:59Z

LTSupportAutomotive/LTOBD2PID.m

@@ -3,6 +3,7 @@
 //

 #import "LTOBD2PID.h"
+#import "float.h"


If that's just for the (double) case, we better #import <Foundation/Foundation.h> here.

In Objective-C-land we usually try to avoid the plain C headers like stdio.h, and friends.

Thanks, will update.

mickeyl · 2021-03-26T09:03:55Z

LTSupportAutomotive/LTOBD2PID.m

@@ -90,6 +91,20 @@ -(instancetype)initWithString:(NSString*)string inFreezeFrameDTC:(LTOBD2DTC*)fre
 #pragma mark -
 #pragma mark API for subclasses

+-(NSNumber*) decodeSingleByteDoubleValueWithOffset:(double)offset factor:(double)factor


Minor nitpick: Coding style on this library is to not have whitespace between the result type and the start of the method name. There's more of that below, will not add individual comments :-)

No problem, happy to align with coding style of the library. Consistency is important, I will fix where I see it.

Ok, think i got all of them

mickeyl · 2021-03-26T09:09:29Z

LTSupportAutomotive/LTOBD2Command.m

+
+/**
+ Units string used when building the formattedResponse
+ Default units is empty string.
+ Subclasses can override to define their own units.
+*/
+-(NSString*)units
+{
+    return @"";
+}
+
+/**
+ Format string used when building the formattedResponse
+ Default format is simple string concatentation
+ Subclasses can override to define their own formatting for integer and double value types.
+*/
+-(NSString*)format
+{
+    return @"%@";
+}
+
+/**
+ Declares the type of the decodedResponse object.
+   - String
+   - Integer
+   - Double
+ Default responseDataType is a String.
+ Subclasses can override to configure their own responseDataType.
+*/
+-(LTResponseDataType)responseDataType
+{
+    return LTString;
+}
+
+/**
+ Default formattedResponse implementation
+ Uses the format string to format the decodeResponse object and then appends the units string.
+ Subclasses can override to customise their own formattedResponse string.
+ However, most commands do not need to customise this function, and can just configure format and units strings and provide an impl of decodeResponse.
+*/
+-(NSString*)formattedResponse
+{
+    if ( !self.responsePayload )
+    {
+        return LTStringLookupWithPlaceholder(@"OBD2_NO_DATA", @"N/A");
+    }
+
+    NSObject* decodedResponse = self.decodeResponse;
+
+    LOG( @"decodedResponse: '%@'", decodedResponse );
+
+    NSString* responseString;
+
+    if (self.responseDataType == LTString) {
+        responseString = [NSString stringWithFormat:self.format, decodedResponse];
+    } else if (self.responseDataType == LTInteger) {
+        int decodedIntValue = ((NSNumber*) decodedResponse).intValue;
+        responseString = [NSString stringWithFormat:self.format, decodedIntValue ];
+    } else if (self.responseDataType == LTDouble) {
+        double decodedDoubleValue = ((NSNumber*) decodedResponse).doubleValue;
+        responseString = [NSString stringWithFormat:self.format, decodedDoubleValue ];
+    } else {
+        LOG( @"ERROR - UNHANDLED ResponseDataType: '%@'", self.responseDataType );
+    }
+
+    LOG( @"responseString after format: '%@': '%@'", self.format, responseString );
+
+    if (self.units) {
+        responseString = [NSString stringWithFormat: @"%@" UTF8_NARROW_NOBREAK_SPACE @"%@", responseString, self.units];
+        LOG( @"responseString with units: '%@'", responseString );
+    }
+
+    return responseString;
+}
+


The introduction of a unit concept is a great change in principle, something I would have wanted to implement right from the start, but never got around to actually doing it – instead we have to live with that inflexible formattedResponse that conveys little semantics.

That said though, if you are to introduce a unit concept, I'd rather see it utilizing the concepts from Foundation, which already contains classes such as NSUnit, NSMeasurement, and NSUnitFormatter.

Sounds like a good idea. I will look at your suggestion.

Mickey, I have updated my changes to use NSUnit, NSMeasurement, and NSUnitFormatter. Will send a PR soon.

mickeyl · 2021-03-26T09:10:20Z

LTSupportAutomotive/LTVIN.m

+    static NSUInteger const MODEL_YEAR_RANGE_DISCRIMINATOR_LOCATION = 6;
+    static NSUInteger const MODEL_YEAR_CODE_LOCATION = 9;
+
+    if (isdigit([_vin characterAtIndex:MODEL_YEAR_RANGE_DISCRIMINATOR_LOCATION])) {
+        // if the character at index 6 is a numeric digit then we select our model year from the 'old' 1980-2009 range
+        NSString* modelYearCode = [@"VIN_MODEL_YEAR_OLD_" stringByAppendingString:[_vin substringWithRange:NSMakeRange(MODEL_YEAR_CODE_LOCATION, 1)]];
+        _modelYear = LTStringLookupWithPlaceholder( modelYearCode, UNASSIGNED );
+    } else {
+        // if the character at index 6 is not a numeric digit then we select our model year from the 'new' 2010-2029 range
+        NSString* modelYearCode = [@"VIN_MODEL_YEAR_NEW_" stringByAppendingString:[_vin substringWithRange:NSMakeRange(MODEL_YEAR_CODE_LOCATION, 1)]];
+        _modelYear = LTStringLookupWithPlaceholder( modelYearCode, UNASSIGNED );
+    }
+


Does that really hold for all kind of VINs?

On Wikipedia page: https://en.wikipedia.org/wiki/Vehicle_identification_number in section "Model Year Encoding" it says that:
On April 30, 2008, the US National Highway Traffic Safety Administration adopted a final rule amending 49 CFR Part 565, "so that the current 17 character vehicle identification number (VIN) system, which has been in place for almost 30 years, can continue in use for at least another 30 years", in the process making several changes to the VIN requirements applicable to all motor vehicles manufactured for sale in the United States:
In order to identify the exact year in passenger cars and multipurpose passenger vehicles with a GVWR of 10,000 or less, one must read position 7 as well as position 10. For passenger cars, and for multipurpose passenger vehicles and trucks with a gross vehicle weight rating of 10,000 lb (4,500 kg) or less, if position seven is numeric, the model year in position 10 of the VIN refers to a year in the range 1980–2009. If position seven is alphabetic, the model year in position 10 of VIN refers to a year in the range 2010–2039.

I cant guarantee this and it probably needs more testing. But without something like this, then the library will return incorrect model year for most recent cars. At some point in time in a modern app we should err on expecting that a car is more likely to be 2010-2039 than be 1980-2009. Tough call. Perhaps you could add support here for a heuristic or algorithm that looked at other elements in the VIN or in the other PIDs to help decide if the car is 1980-2009 or 2010-2039.

I agree. The code is fine and will improve things. Given also that this VIN class is probably most often used to identify cars attached to an OBD2 adapter, the model year almost always has to be above 1996.

mlawlerau · 2021-03-26T18:17:49Z

No worries, we are all busy - this is not my full-time job either. Completely agree that more fine grained commits is preferred - will try to do this. Had a quick scan of your comments - look good, and will respond in detail. Happy to be advised on style and C++ / Objective C / iOS style - I am coming from 20 years in enterprise java land. I’m passionate about getting it right and making it understandable by others, so happy to iterate on contributions.

…

On 26 Mar 2021, at 08:53, Dr. Mickey Lauer ***@***.***> wrote: First off, sorry for the delay in answering. I'm super busy with a new project (more on that later, but it has to do with car diagnostics). Thanks a lot for your pull request. I will start commenting on the individual parts of the pull request in detail right after finishing this message. In general, what we should aim for is more fine granular commits, so that we have a chance to retrace the history. Please see the inline comments. Looking forward to merging this contribution! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#36 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKN7RYPAFLIEUOXIBSQYO23TFRDRHANCNFSM4YBCRXNA>.

mlawlerau added 2 commits February 22, 2021 17:31

mickeyl reviewed Mar 26, 2021

View reviewed changes

mlawlerau marked this pull request as draft April 19, 2021 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft for Discussion: mlawlerau #36

Draft for Discussion: mlawlerau #36

mlawlerau commented Feb 22, 2021

mickeyl commented Mar 26, 2021

mickeyl Mar 26, 2021

mlawlerau Apr 19, 2021

mickeyl Mar 26, 2021

mickeyl Mar 26, 2021

mlawlerau Apr 19, 2021

mickeyl Mar 26, 2021

mlawlerau Apr 19, 2021

mickeyl Mar 26, 2021 •

edited

Loading

mlawlerau Apr 19, 2021

mlawlerau Apr 19, 2021

mickeyl Mar 26, 2021

mlawlerau Apr 19, 2021

mlawlerau May 10, 2021

mickeyl Mar 26, 2021

mlawlerau Apr 19, 2021

mlawlerau Apr 19, 2021

mickeyl Apr 21, 2021 •

edited

Loading

mlawlerau commented Mar 26, 2021 via email

Draft for Discussion: mlawlerau #36

Are you sure you want to change the base?

Draft for Discussion: mlawlerau #36

Conversation

mlawlerau commented Feb 22, 2021

mickeyl commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickeyl Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickeyl Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

mlawlerau commented Mar 26, 2021 via email

mickeyl Mar 26, 2021 •

edited

Loading

mickeyl Apr 21, 2021 •

edited

Loading