Everything is written to the file from the thread the method was called from. This could create threading issues with the file, and if written on the main queue, we're now writing to disk from the UI queue, which should be avoided.

I'd propose we create a serial dispatch queue that can write to the file asynchronously, to not block the thread we're logging from, and to prevent threading issues.

Let's Unit Test all of our public API methods. This will help with future changes, and also with open source, since all open source changes can be tested for breakage and add additional tests for new features.

Foreground app tracking requires the ability to send RPC Notifications to the head unit, we may as well include the ability to send any type of message. This issue will be resolved as part of the PR #33.

Is there a matrix showing features of SDL and what vehicles/models support which features (and over which transport)?

https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow

Hi,

I assume that this issue tracker, here at GitHub, is the authoritative issue tracker for SDL. I say that because there are some lingering issues in GENIVI's bug tracker and I don't want them to get lost and lonely over there: http://bugs.genivi.org/buglist.cgi?query_format=specific&order=relevance%20desc&bug_status=__open__&product=SmartDeviceLink&list_id=2505

It likely wouldn't take much to move them here, if that's needed. Or, if they're resolved, they can get closed.

Thanks!

The current precompiled header includes UIKit, which is not a framework needed to be imported and accessible in most classes in SDL so it should be removed.

The precompiled header already imports Foundation so Foundation imports can be removed elsewhere.

Anything that is imported in most classes should be moved to the precompiled header instead.

It's currently sandwiched in the SDLRPCMessage file.

Which means when comparing we're just comparing pointers. This works right now because we're using global objects, but it's bad practice, and resistant to testing and refactoring.

It's the only enum i've seen that doesn't use constants

Class forwarding (@class) syntax is used almost nowhere in the project. Many .h import statements can be altered to @class statements. This should help prevent import loops and reduce compile times.

Xcode 6.3 introduces attributes for parameters and properties that indicate nullability, mainly for Swift interop (EDIT: Also for Obj-C interactions!). We'll want to implement that into our APIs to help any Swift developers out there.

 #pragma clang assume_nonnull begin
     // ...
     -(void)registerNib:(UINib *)nib forCellReuseIdentifier:(NSString
*)identifier;
-(nullable UITableViewCell *)cellForRowAtIndexPath:(NSIndexPath)indexPath;
@property (nonatomic, readwrite, retain, nullable) UIView *backgroundView; // ...
#pragma clang assume_nonnull end

Adding nullability annotations to Objective-C APIs does not affect backward compatibility or the way in which the compiler generates code. For example, nonnull pointers can still end up being nil in some cases, such as when messaging a nil receiver. However, nullability annotations—in addition to improving the experience in Swift—provide new warnings in Objective-C if, for example, a nil argument is passed to a nonnull parameter, making Objective-C APIs more expressive and easier to use correctly.

The I syntax is a Java platform syntax. The Cocoa syntax would be SDLProxyDelegate.

https://github.com/smartdevicelink/sdl_ios/blob/05796f6810d54dafecdf3114da82ef6b2c7e3f33/sdl_ios/SmartDeviceLink/ISDLProxy.h

The method is currently defined as

- (void)putFileStream:(NSInputStream*)inputStream :(SDLPutFile*)putFileRPCRequest;

and should be something like

- (void)putFileStream:(NSInputStream*)inputStream withRequest:(SDLPutFile*)putFileRPCRequest;

Note: the current method may need to be deprecated in order to avoid compatibility issues and increasing the major version number.

currently defined as

+(NSString*) NEGATIVE_JINGLE { return @"NEGATIVE_JINGLE"; }
+(NSString*) POSITIVE_JINGLE { return @"POSITIVE_JINGLE"; }
+(NSString*) LISTEN_JINGLE { return @"LISTEN_JINGLE"; }
+(NSString*) INITIAL_JINGLE { return @"INITIAL_JINGLE"; }
+(NSString*) HELP_JINGLE { return @"HELP_JINGLE"; }

see #3 for more information

See #21

SDLJsonEncoder and Decoder classes add unnecessary complexity and improperly built singletons to call a single line of code

NSData* jsonData = [NSJSONSerialization dataWithJSONObject:dict options:kNilOptions error:&error];

The error is ignored in both cases. It also defines two protocols, that are only used in those classes (Defined in SDLDecoder.h and SDLEncoder.h)

SDLDecoder.h and SDLJsonDecoder can be removed and the line

NSDictionary* rpcMessageAsDictionary = [[SDLJsonDecoder instance] decode:self.payload];

can be replaced in SDLV1ProtocolMessage and SDLV2ProtocolMessage with

NSDictionary *jsonObject = [NSJSONSerialization JSONObjectWithData:msgBytes options:kNilOptions error:nil];

The same can be applied to SDLJsonEncoder, and the one class which calls its encode message

Every setter method in every SDLRPCStruct subclass takes an object and sets it in the store. If the object passed into the method is nil, the object is removed from the store instead (because passing nil to setObject throws an NSInvalidArgumentException). This makes every setter look like

-(void) setNavigationText:(NSString*) navigationText {
    if (navigationText != nil) {
        [store setObject:navigationText forKey:NAMES_navigationText];
    } else {
        [store removeObjectForKey:NAMES_navigationText];
    }
}

This can be cleaned up by creating a category on NSMutableDictionary and including it in the SDLRPCStruct class. It would look like

- (void)setOrRemoveObject:(id)object forKey:(id <NSCopying>)key {
    if (nil != object) {
        [self setObject:object forKey:key];
    } else {
        [self removeObjectForKey:key];
    }
}

Then every setter can be reduced to

- (void)setNavigationText:(NSString *)navigationText {
    [store setOrRemoveObject:navigationText forKey:NAMES_navigationText];
}

It can be safely removed

Not having any sort of threading is a separate (larger) issue, but that every property is atomic is going to slow the library down quite a bit. Most (or all, if we do our threading properly) things should be nonatomic.

I haven't looked deep into this, so I don't know if it's indicative of a larger issue (such as running the socket connection on the main thread). Either way, this should not block the main thread, but should have callbacks of some sort to indicate when a connection or timeout occurs.

The parameter key and value constants defined in SDLNames.h should use extern instead of define for safe compile time type checking and to avoid collisions.

Apple's Recommendation suggests

Define constants for strings used for such purposes as notification names and dictionary keys. By using string constants, you are ensuring that the compiler verifies the proper value is specified (that is, it performs spell checking).

Prefer properties everywhere.

For instance SDLProxy's private instance variable SDLLockScreenManager should be a private property.

SDLProtocol.m

The current process is:

1. Receive Bytes
2. Check buffer for header length
3. Process header
4. Check buffer for message length
5. Process message
6. Remove header and message from buffer
7. Route the message

Every time the full message is not in the buffer and bytes are received the header is re-parsed. A state machine should be used to parse the incoming bytes.

Especially in the case of the putFileRPCRequest, which is already retained as an object association by the inputStream.

The only time this property is retrieved is in the proxy's [stream:handleEvent] method, which has access to the putFileRPCRequest which computes the property.

From Apple

Although Objective-C includes syntax for exception handling, Cocoa and Cocoa Touch use exceptions only for programming errors (such as out of bounds array access), which should be fixed before an app is shipped.
All other errors—including runtime problems such as running out of disk space or not being able to access a web service—are represented by instances of the NSError class. Your app should plan for errors and decide how best to handle them in order to present the best possible user experience when something goes wrong.

I've noticed a few places in the proxy where exceptions are used as flow control, those should be refactored.

This is completely unnecessary, it's importing a ton of code here for no reason.

The library is in a state at this point that it is inevitable that we will be changing public apis, as nearly every API is public. This is not ideal for the semantic versioning structure currently employed by this project.

However, there is a loophole. Point 4 of Semver states:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

I realize that version 3.0.0 is where we started the project, but I believe the state of the project is really a version zero state.

So I propose that we change our versioning, existing tags will be shifted to be v0.1.0, v0.1.1, and v0.1.2 respectively. At such a time that the API is stable, we can revisit tagging the project as v1.0.0.

Note: By project, I mean the iOS Proxy, not Core, the Protocol, or any other component of SmartDeviceLink.

Currently most code is styled incorrectly according to Apple's Coding Guidelines for Cocoa.

I also really like the New York Times Objective-C Style Guile with only a few disagreements.

Regardless of the type of message, if an SDLRPCMessage object is initialize using initWithName, then the messageType is automatically set to "request".

messageType = NAMES_request;

This should instead set the messageType to whatever is appropriate based on the message.

A simple way of doing this would to override the initWithName function in SDLRPCRequest, SDLRPCNotification, and SDKRPCResponse, and set the messageType in that override instead.

For example, the override in RPCNotification might look like:

-(id) initWithName:(NSString*) name {
    if (self = [super initWithName:name]) {
        messageType = NAMES_notification;
        [store setObject:function forKey:messageType];
    }
    return self;
}

Is it possible to poll/or get notified when a vehicle is in motion?

https://github.com/blog/1184-contributing-guidelines

For instance -[SDLBitsPerSample _8_Bit] should be -[SDLBitsPerSample 8_BIT]

Lots of enum objects permanently in memory now, could do conversion between String Enum <–> Integer Enum instead. Less memory overhead, probably more CPU overhead, likely negligible. Removes an enormous amount of code.

Right now, Enums are designed as objects. From what I understand, there are a few reasons for this:

1. Namespacing. You can type SDLAmbientLightStatus and get a list of the component enums.
2. Typechecking. You can call the [valueOf:] method on an enum and get the object, or nil if it is not a component object.

See the comment below for more details.

Also, see #425, perhaps just typedef EnumName NSString and stringly typed constants?

Tutorial on HeaderDoc
Jazzy

We should document all of our methods next to the methods in header files. This will provide us with in Xcode documentation generation for alt-clicking on methods; as well as the opportunity to generate html based on the documentation.

38 times it's spelled 'recieve'. To fix this will be backward incompatible, because it's in class names and file names.

Currently they return mutable arrays, which means that anybody could accidentally modify the array, which could easily result in bugs.

The initWithDictionary function in the SDLRPCMessage class assumes that whatever dictionary is passed in is in a specific format.

As a result, if a dictionary that doesn't follow this format is passed in, then the function will raise an exception which means nothing to the user.

For Example, this exception is thrown when a dictionary containing a single key:value pair is given to the function:

"failed: caught "NSInvalidArgumentException", "-[SDLAppInterfaceUnregisteredReason objectForKey:]: unrecognized selector sent to instance 0x7f9dc349e9e0""

The function should check whether the dictionary follows the correct format, and return nil or throw an exception describing the problem if it does not.

https://github.com/smartdevicelink/sdl_ios/blob/master/sdl_ios/SmartDeviceLink/SDLProxy.me

Can this be removed?

The values defined in SDLHMILevel all use a different name for the function attached to them from the value that they return. They instead use the internal_name defined for them in the spec, and as a result all of the function names have an "HMI_" prefix attached to them.

+(SDLHMILevel*) HMI_FULL;
+(SDLHMILevel*) HMI_LIMITED;
+(SDLHMILevel*) HMI_BACKGROUND;
+(SDLHMILevel*) HMI_NONE;

This prefix was supposedly necessary at one point in time, but at this point is the only SDLEnum which follows this naming convention, all others enum values with prefixes in their internal_name instead use the name field for their function names.

Developer feedback: "The file is actually named FMCHMIZoneCapabilities.h (note the difference in casing), which causes the build to fail on case-sensitive file systems."

iOS8 added the ability to create dynamic frameworks - building a Cocoa Touch Framework for developers allows them to use the SDL framework in the exact same way that they use native frameworks.

In the code, the raw values are strings: request, response, and notification. However, in the spec, they are defined as 0, 1, and 2 respectively. PR #33 will cover this issue. A standard NS_ENUM will be created for this case.

Since the project is open source, they can dig in to the meat of the library and compile it themselves if they need to. Hiding files that integrators don't need is a great way to help out developers who are getting started with the library review only what is necessary for them to use the library.

To do this, the compile settings for each "private" header file will be changed from public to either project or private (I don't currently remember what the difference is, but we did this with Livio Connect, so it would be easy to check). Then, you have to make sure that all the header files that will be public contain no imports of private header files, and only use the @class tag. Private header files can be included in .m files.

There are several enum values which are defined in the spec, but are not currently implemented in the library

Here is a list of all such values:

1. SDLAppInterfaceUnregisteredReason
   • USER_EXIT
   • PROTOCOL_VIOLATION
2. SDLImageFieldName
   • locationImage
3. SDLResult
   • CHAR_LIMIT_EXCEEDED
4. SDLTextFieldName
   • timeToDestination
   • navigationText
   • notificationText
   • turnText

From what I understand, there are several reasons for this:

1. Some of them have been deprecated and have just not been marked as such (ex. USER_EXIT and navigationText).
2. Some are just related to new features that are in the proxy code, but are not made visible to developers as of yet.
3. Some are new additions which have not yet been implemented at all.

I am not entirely certain which values fall under which category, so if someone more familiar with what features have yet to be implemented could have a look at these, then that would be helpful.

Generally determinism is better. What's up with the randomness (between 0 and 0.5 sec)? Why not just implement a single retry delay?

//Begin Connection Retry
                float randomNumber = (float)arc4random() / UINT_MAX; // between 0 and 1
                float randomMinMax = 0.0f + (0.5f-0.0f)*randomNumber; // between Min (0.0) and Max (0.5)