I²C Proof-of-concept

[NoeelMoeskops]

Proof-of-concept I²C support using libmpsse. This branch is not yet ready to merge. Since it is not yet feature complete and has a lot of not-DRY code. I kinda hacked it together after making #2. Also, I barely tested it since the I²C device I initially wanted to debug is not responding (see screenshot below).

Its missing documentation and better integration with the existing code, since I think that functions like parcecli() should be shared between spi and i2cand the command line argument (cli_cmd_t) for i2c and spi are very similar only address and register is new, but is resulting now in a lot of double code. I already moved some code from spi.c to mpsse-cli.c/h because it can be used by both spi and i2c, thus keeping the code base pretty :).

I would like to get some feedback on the current state (aside from the obvious above and the //TODOs that are already in the code).

Usage:

# ./mpsse-cli i2c -h
usage: mpsse-cli i2c [...option]

options:
    -h, --help - Displays this help menu.
    -c, --channel - MPSSE Channel #. (Available channels can be retrieved with mpsse-cli -l)
    -f, --freq - SPI Frequency
    -x, --xfer - Set transfer type. [r | w]
    -l, --len - Number of bytes to read/write
    -d, --data - Data to be written. Command delimeted list in hex. example: 'mpsse-cli spi -c 0 -x w -l 8 -d 0xDD,0xEE,0xAA,0xDD,0xBB,0xEE,0xEE,0xFF'
    -a, --address - I²C address to read/write to
    -r, --register - I²C register to read/write

Example:

# mpsse-cli i2c -x r -a 0x40 -r 0x00
No response from I²C slave.

You need to install both libmpsse and ftdi1 in order to compile and run the code.

[stephendpmurphy]

Looks OK so far. As you already stated, there is a lot of reuse that can be put into a single "util" like C file. Things like the comma delimited data parsing, CLI options printing, etc.

You may notice, I'm also a real stickler on comments. So as we get closer to a PR ready state, I would like to see things explained more clearly in the functions.

The last bit is just integrating the open-source lib. I would potentially like to switch the SPI portion over to the open-source API with it statically compiled in FIRST, and that way this portion can merge and be immediately functional on the develop branch.

Let me know your thoughts.

[NoeelMoeskops]

Looks OK so far. As you already stated, there is a lot of reuse that can be put into a single "util" like C file. Things like the comma delimited data parsing, CLI options printing, etc.

I gave it some thought and maybe it would be a good strategy to convert the application to c++. That way all the modules (spi, I²C, GPIO etc.) can inherit a base class that takes care of handling cli and have shared code. I have not thought of an exact architecture yet, because first I want to know if you are open to the idea. Also, maybe we can use an already existing library for handling cli parsing. That way a lot of edge cases are handled, and we can focus on mpsse integration!

You may notice, I'm also a real stickler on comments. So as we get closer to a PR ready state, I would like to see things explained more clearly in the functions.

👍

The last bit is just integrating the open-source lib. I would potentially like to switch the SPI portion over to the open-source API with it statically compiled in FIRST, and that way this portion can merge and be immediately functional on the develop branch.

Yes, I am glad you agreed on the opensource libs. Having two libraries in one project that does exactly the same is kinda odd indeed. I will write up the I²C part, that way you can maybe focus on converting spi to libmpsse? A lot of the main mpsse-cli.c also needs to be converted since its using ft2xx as well.

[NoeelMoeskops]

I cleaned up the code and moved all the CLI parsing and handling to a single file cli.c/h. The only consequence of this action is that the help menu is more generic and is identical for all subcommands. But the code is much more readable, less error-prone and easier to adjust because previously the cli handling was done at separate stages throughout the code. Now only main() parses the given commands, put them in a struct and calls the correct function, with that function not having to worry about argc or argv. Please see the code of commit 56676931cc10966ab5a783dc4c12083a2930b2ff.

As you can see the size of spi.c and i2c.c have shrunk significantly. Since now, they only really contain read and write functionality. If we were to switch to c++ we could make a base class with abstract read() and write() functions. That way we can ensure that i2c and spi (and other to come possible) behave the same regarding the commands given and only differ in the (hardware) execution of the write/read functionality!

[stephendpmurphy]

I gave it some thought and maybe it would be a good strategy to convert the application to c++. That way all the modules (spi, I²C, GPIO etc.) can inherit a base class that takes care of handling cli and have shared code. I have not thought of an exact architecture yet, because first I want to know if you are open to the idea. Also, maybe we can use an already existing library for handling cli parsing. That way a lot of edge cases are handled, and we can focus on mpsse integration!

I'll be honest I BARELY know C++, but I'm more than happy to learn (I've always used C and have never had a great reason to switch). I'm fluent in OOP and have used JS, Java, and C# extensively. I just need to pick up the syntax for C++. I completely understand the benefits though. Let's do it.

I cleaned up the code and moved all the CLI parsing and handling to a single file cli.c/h. The only consequence of this action is that the help menu is more generic and is identical for all subcommands. But the code is much more readable, less error-prone and easier to adjust because previously the cli handling was done at separate stages throughout the code.

Awesome. I agree. I read you made a comment on trying to find an alternate parsing method that is not in this PR. But regardless. See issue #6. I think it would be good to adopt a "standard" method for parsing instead of spinning our own.

Yes, I am glad you agreed on the opensource libs. Having two libraries in one project that does exactly the same is kinda odd indeed. I will write up the I²C part, that way you can maybe focus on converting spi to libmpsse? A lot of the main mpsse-cli.c also needs to be converted since its using ft2xx as well.

I'll gladly take this one. It will also help me become familiar with the new lib APIs. I apologize I may block you up for a bit on this PR. I have gaps where I can dedicate a lot of time to these projects and then I get busy with work. I anticipate this week to be busy since I need to catch up from vacation. I'll try my best though.

Creating issue #9

[NoeelMoeskops]

I'll be honest I BARELY know C++, but I'm more than happy to learn (I've always used C and have never had a great reason to switch). I'm fluent in OOP and have used JS, Java, and C# extensively. I just need to pick up the syntax for C++. I completely understand the benefits though. Let's do it.

Alright! Do you mind me creating the CPP classes and structures in this branch? Otherwise, I would have to do a lot of git voodoo. Or do you rather want to create the setup? Either way I think the best course of action is to first create the CPP class structure and then the I²C functionality, because otherwise it would be a bit of double work.

[stephendpmurphy]

Do what works best and makes the most sense. Even if that means switching over in this branch. That way it will also give me something to base the branch I will be switching SPI to the new FTDI lib on.

[stephendpmurphy]

Setting PR to a draft for the moment.

[NoeelMoeskops]

@stephendpmurphy I just pushed the C++ changes. Now both I²C and SPI are object based and a parent of the Protocol class. All the functionality of printing, checking the input etc. is done in the main() while only the protocol specific things are done in the respectable classes using the _read/write() function. I also added exceptions since that is less error-prone than returning error codes :). Please take a look, and hopefully we can merge with develop because now that you started to work on the CLI I am afraid it will get too segmented otherwise 👍.

[stephendpmurphy]

FYI @NoeelMoeskops well on my way to having the CLI implementation done. I went ahead and merged this I2C branch into my branch and it connected up nicely :+1: again, good work on the abstraction and architecture. Just need to get the above feedback made and merged into develop and then I can push the CLI revamp.

[stephendpmurphy]

I'm gonna go ahead and merge this just to keep the workflow going. I'll make issues to address the remarks above.