> First, it is kind of awkward that I'm working with a "random" branch
> of xpcc, in that I don't know where to apply bugfixes (I found a few
> minor ones) so that they get back to my branch. Should I fork xpcc and
> create a new "working" branch from master, and merge in the stm32f103
> support branch and then later the new commits from master, and publish
> the bug fixes on xpcc:master? (so if a new commit arrives to master, I
> can merge it back to my branch)

The 103 branch that you are on is only required for SystemClock and 
linkerscript support, it doesn't do anything else.
It builds on feature/system_clock which will probably not be merged in the near 
future since a lot more research into clock tree abtraction needs to be done.

We should backport the required specific system clock tree for the F103 though 
and merge the support into develop (not master!).
Our master branch is old, I'm not even sure why we still have it.
We are lacking a release strategy.

Your F103 branch is up to date with develop, so you should commit your bugfixes 
on top of it and then cherry-pick them to a seperate feature/bugfix branch on 
top of develop.
Then make a Pull Request for each one of them for Code Review.
So yes, you need to fork.

To be clear, I don't intend to merge the F103 branch into develop, since it 
would mean merging the unfinished feature/system_clock branch as well.
Unfortunately the system clock thing is a lot more complicated than I thought, 
so sorry for the complicated cherry picking approach.

> Second, I noticed that on the I2cTransaction class, which is supposed
> to be a general abstract class of every transaction, there are
> "configureWriteRead", "configureWrite" and "configureRead", which
> doesn't make sense to me (they aren't even implemented). Also, of the
> 3 classes which implement these all have all 3 methods even when it
> doesn't make any sense (like "configureRead" on
> "I2cWriteTransaction")

Ah, you arrived in the belly of the beast.

The I2cMaster delegates I2C state machine decisions to the I2cTransaction via 
the virtual callback methods.
The `configure*` methods should be pure virtual too, as they are required by 
I2cDevice in its `start*` methods.
However, since the class type is passed by template anyway, these methods do 
not need to be virtual, because the derived class type is known and therefore 
used _directly_.
The methods only exist in the base class for documentation.

Since I2cDevice is the same for every I2CTransaction type, it uses the methods 
`configureRead` to provide `startRead` even if it makes no sense for this 
Thats why they are protected, the driver inheriting from this class needs to 
abstract this away anyway.

Have a look at the concept description for I2C for the state machine graph 
(ignore the SPI stuff, we do that differently now):
(there is a rendered PDF under concurrency-modelling/document, page 11 has the 
graph, page 6 describes transaction objects).

> Now, I figured I would write a transaction that would be
> register-oriented (first byte of data is register number, then read or
> write). On this transaction, the aforementioned methods should simply
> not exist, but that's currently impossible.

The I2cTransaction describes the I2C state machine, but shouldn't know anything 
else about the semantics of what you are doing.
So, you _could_ do the register abstraction directly in a custom 
I2cTransaction, but I _really don't recommend_ it.

The only two devices that use a custom I2cTransaction are the SSD1306 OLED 
display (multiple bus transfers in one transaction) and a generic eeprom driver.
The latter requires a start-write(16bit address)-write(data)-stop, on 
non-continous memory so that the storage data does not have to be copied behind 
the 16bit address.
So here a custom I2c state machine was required, but the use of these special 
transactions are very rare.

The other I2c drivers already use the register-oriented concept and expand on 
it by abstracting away the transport layer altogether.
Please use the WriteReadTransaction and build on top of that.

I recommend looking at drivers like the ITG3200 gyro, HMC58x3 magnetometer 
which use this transaction and register abstraction extensively.
(and perhaps Lsm303a accelerometer for transport layer abstraction).

There is an entire blog post on register abstraction on blog.xpcc.io.

> Third, I wonder, why is "draw" a function pointer in "GraphicDisplay"
> and not a virtual function? I couldn't yet figure it out...

`draw` points to either setPixel or clearPixel (both virtual), so that the 
actual drawing method (for example Bresenham line interpolation) only has to 
choose the pixel and not worry about what to do with it.

Please note, that the entire graphics subsystem was written for small binary 
displays driven by AVRs, and color and GUI was sort of added as an afterthought 
when we needed it.
I'm working on an entirely new graphic system for Cortex-M over here: 

Keep on hacking, you're very attentive and we need people like you for xpcc!

Happy New Year,
xpcc-dev mailing list

Reply via email to