M308 and Current Loop Temperature Sensors

AJ Quick

I don't think the parent / child would be a bad idea. However, I am not sure exactly how it would be best to implement.

Perhaps if we had "current-loop" as one option and then "current-loop-child" as another?

Would it make sense to assign the channel through the "P" command?

My understanding is it could be assigned as follows:

M308 S0 P"spi.cs5" Y"current-loop" A"Channel 0" ; Current Loop Sensor by default on Channel 0.
M308 S1 P"S0.1" Y"current-loop-child" A"Channel 1" ; Current Loop Child on Channel 1
M308 S2 P"S0.7" Y"current-loop-child" A"Channel 7" ; Current Loop Child on Channel 7

OR

M308 S0 P"spi.cs5" Y"current-loop" A"Channel 7" C7 ; Current Loop Sensor on Channel 7.
M308 S1 P"S0.1" Y"current-loop-child" A"Channel 5" C5 ; Current Loop Child on Channel 5
M308 S2 P"S0.2" Y"current-loop-child" A"Channel 6" C6 ; Current Loop Child on Channel 6

I think the second example is better as not everyone is going to automatically be using Channel 0. Perhaps this can be done without needing to call it 'current-loop-child' but instead just checking if the pin is already in use by a current loop sensor? If it is in use, then apply the 'AdditionalOutputSensor'?

AJ Quick

Unsurprisingly, my possible fix (M308 and Current Loop Temperature Sensors) did not work.

I did not understand how the port was being assigned correctly.

I'll look more into this parent / child relationship and see what I can accomplish.

dc42

@AJ-Quick said in M308 and Current Loop Temperature Sensors:

I don't think the parent / child would be a bad idea. However, I am not sure exactly how it would be best to implement.

Perhaps if we had "current-loop" as one option and then "current-loop-child" as another?

Would it make sense to assign the channel through the "P" command?

My understanding is it could be assigned as follows:
M308 S0 P"spi.cs5" Y"current-loop" A"Channel 0" ; Current Loop Sensor by default on Channel 0.
M308 S1 P"S0.1" Y"current-loop-child" A"Channel 1" ; Current Loop Child on Channel 1
M308 S2 P"S0.7" Y"current-loop-child" A"Channel 7" ; Current Loop Child on Channel 7
OR
M308 S0 P"spi.cs5" Y"current-loop" A"Channel 7" C7 ; Current Loop Sensor on Channel 7.
M308 S1 P"S0.1" Y"current-loop-child" A"Channel 5" C5 ; Current Loop Child on Channel 5
M308 S2 P"S0.2" Y"current-loop-child" A"Channel 6" C6 ; Current Loop Child on Channel 6
I think the second example is better as not everyone is going to automatically be using Channel 0. Perhaps this can be done without needing to call it 'current-loop-child' but instead just checking if the pin is already in use by a current loop sensor? If it is in use, then apply the 'AdditionalOutputSensor'?

The first option would fit better with the architecture and I think it would be easier to implement.

AJ Quick

@dc42 said in M308 and Current Loop Temperature Sensors:

The first option would fit better with the architecture and I think it would be easier to implement.

Do you believe it could be done without calling it 'current-loop-child' (or a variant)? Or would something need to be made to differentiate it from a regular current-loop sensor?

dc42

@AJ-Quick said in M308 and Current Loop Temperature Sensors:

Do you believe it could be done without calling it 'current-loop-child' (or a variant)? Or would something need to be made to differentiate it from a regular current-loop sensor?

Yes, it needs a different sensor type name from the sensor that it piggybacks on to, because the name identifies which driver class it uses. Could be current-loop-additional or current-loop-extra or something like that. I suggest you keep "current-loop" in the name to make it clear which master sensor type it needs.

AJ Quick

I made the following changes:

https://github.com/dc42/RepRapFirmware/pull/380

I can't imagine it is that easy though. This does add 'current-loop-extra' which then (hopefully) directs to AdditionalOutputSensor. I am assuming all parameters that are passed, won't be passed to the parent though?

So if the "C" parameter is working now, it should work to set the different "C" parameter on the child? It won't force the channel to be adopted by the parent?

This possibly works like this now:

M308 S0 P"spi.cs5" Y"current-loop" A"Channel 7" C7 ; Current Loop Sensor on Channel 7.
M308 S1 P"S0.1" Y"current-loop-extra" A"Channel 5" C5 ; Current Loop Child on Channel 5
M308 S2 P"S0.2" Y"current-loop-extra" A"Channel 6" C6 ; Current Loop Child on Channel 6

Up to 7 children can be defined.

In order for the P parameter to carry the channel data, the existing code would need to be changed.

ajquick opened this pull request in dc42/RepRapFirmware

draft CurrentLoopSensors can now have children sensors. #380

dc42

Do you need the C parameter at all? I would have thought that the master current loop sensor would use channel 0, then child 1 (M308 S2 P"S0.1") would use channel 1, and so on.

AJ Quick

@dc42 Yes.

While channel 0 is the default, you can't assume all end users will have their main temperature probe set on channel 0. My main temperature reading is on channel 2 for example.

To force the channel to be designated by the P parameter, you lose backwards compatibility and need to change more of the default CurrentLoop code. Since you need to specify L and H settings anyways, it makes sense to just continue to use the C (and D) parameters for the child as well.

AJ Quick

Current status:

Current-Loop-Extra currently works by assigning a child of the parent sensor. Unfortunately the child is an exact clone of the parent. No channel, differential or low and high parameters are passed.

This might just be a problem with "AdditionalOutputSensor.cpp" though. It probably only passes the "Y" parameter currently.

C/D/L/H cannot be assumed to be the same for the parent and the child. For example my thermocouples have different L/H ranges.

AJ Quick

At a total loss here.

I am now understanding how the DHT sensor works, and it is not the same as how the MCP3204/8 works. DHT seems to always have the humidity value present, so it is a simple matter to just ask what that value is.

MCP3204/8 needs to treat each sensor as their own and communicate with the chip for each and every one. It also isn't something you can just change the channel and grab, since each sensor has its settings. I tried to allow AdditionalOutputSensor to configure the child sensor, but it just overrides the parent.

I suppose it would be possible to fully expand the CurrentLoopExtraSensor class to basically be a copy of CurrentLoopSensor, but that's not clean. There would be lots of code re-use.

The best thing I think that can be done is just to allow the SPI chipselect to be used by multiple sensors as long as the sensor is of the same type. At this time, I do not know how to accomplish that. I believe it should be safe to re-use chipselect pins, because they will automatically be queued and wouldn't be talking over each other. The worst case scenario is someone puts in the wrong chipselect and it cannot connect.

AJ Quick

This is what I've come up with:

https://github.com/dc42/RepRapFirmware/pull/380/commits/2ca324d3fd53f2988b4e2d95e878a2347b4065ee

Basically, add a new pin type of "chipSelect".

If it is of type chipSelect, it can be shared. When an SPI sensor makes a port, it assigns as type chipSelect. Now only other sensors can use that port for chip-select if it was previously used for chip-select. It should throw an error if any other code tries to use it for something other than chip-select.

This should allow CurrentLoopSensor to work while sharing the same chip-select and potentially can be used for other chips down the line.

ajquick opened this pull request in dc42/RepRapFirmware

draft CurrentLoopSensors can now have children sensors. #380

AJ Quick

Testing now. Those changes work, however any sensor that comes after the first are not reading the correct information.

M308 S0: Sensor 0 (Chamber) type Current Loop using pin (spi.cs5,duex.cs5,exp.50), reading 41.7, last error: success, channel: 2, temperature range 0.0 to 100.0C.
M308 S1: Sensor 1 (Model) type Current Loop using pin (spi.cs5,duex.cs5,exp.50), reading 201.6, last error: success, channel: 0, temperature range 109.5 to 330.0C
M308 S2: Sensor 2 (Support) type Current Loop using pin (spi.cs5,duex.cs5,exp.50), reading 201.4, last error: success, channel: 1, temperature range 109.5 to 330.0C

If I disable all except the Model for example. The temperature changes to the correct reading:

M308 S1: Sensor 1 (Model) type Current Loop using pin (spi.cs5,duex.cs5,exp.50), reading 109.5, last error: success, channel: 0, temperature range 109.5 to 330.0C

Seems the first sensor defined is always reading the correct number, but the 2nd or 3rd is always reading the wrong number. I suppose the SPI isn't actually queuing reads correctly? Further testing is needed.

dc42

@AJ-Quick said in M308 and Current Loop Temperature Sensors:

I suppose the SPI isn't actually queuing reads correctly?

I can't remember how the MCP3204 works, however with many SPI devices the data you read during a transaction is from the register whose address you set in the previous transaction.

It would probably be better to have the master device read the ADC results for all channels (which can probably be done in a single transaction - check the datasheet) and store them; then each child device can ask the master for the reading from its own channel. If you want the child devices to have different ranges, then either the child device will need its own configuration parameters, or the master configuration parameters changed to allow different ones to be specified for each channel (for example, by accepting arrays for the H and L parameters).

AJ Quick

@dc42

Best I can tell, only one channel can be read at a time.

Theoretically, if you have two CurrentLoopSensors running on different chip select pins, wouldn't it schedule the SPI transactions so that they are never talking over each other?

It is odd that the first channel to be designated is always correct, the 2nd, 3rd, 4th.. etc. Are always tied to each other and produce the wrong data. Shouldn't they all be separated by the different instances of the sensor? Shouldn't the SPI transaction be treating each one as if it were it's own sensor running on SPI and keep them separated? I imagine it could be an SPI bug.

Here is what I did to test:

I changed my code to have 3 CurrentLoopSensors' each using their own chipSelect pin. I tied all 3 chip select pins together, so it doesn't really matter who is driving the signal. The same behavior exists no matter if it is using the code I just changed or the default RC8 firmware. It doesn't keep the SPI sensors separate.

EDIT: Just found what is going on:

It is only producing the ADC reading from the first defined sensor, it is just on the L/H scale that is specified.

AJ Quick

My best guess at what is happening is this:

Everything with the CurrentLoopSensor is configured properly.

-When CurrentLoopSensor issues SPI read command it is sent over the SpiTemperatureSensor.
-SpiTemperatureSensor requests a lock to use SPI with-in 10 (attempts? seconds? milliseconds?)
-If lock is successful, it sends the data out and receives the temperature data back.
-The temperature data that is sent back is always for the first defined sensor in the config, despite the data out presumably being correct.
-The data is correctly interpreted based on the Low / High settings and computed into a temperature.

Something in there is causing the SPI data to always come back as the first defined sensor. Perhaps the SPI lock isn't actually working? Are the other sensors trying to talk around the same time as the first sensor? It's reading the first sensor's data as if it were the data it had requested?

I'm assuming it will poll in order: Sensor 1 > 2 > 3, so Sensor 2 and 3 are seeing the data for Sensor 1 and assuming it is their own?

dc42

I think the problem may be the "static" keyword at line 117 of CurrentLoopTemperatureSensor.cpp. I will remove it.

PS - the latest RepRapFirmware commits depend on updates to CoreNG and FreeRTOS that I have also committed.

AJ Quick

@dc42 said in M308 and Current Loop Temperature Sensors:

I think the problem may be the "static" keyword at line 117 of CurrentLoopTemperatureSensor.cpp. I will remove it.

Bingo. That was it!

AJ Quick

My code for re-using the chip-select pin between multiple SPI sensors is now ready as well:

https://github.com/dc42/RepRapFirmware/pull/383/files

This change is simple and is forwards / backwards compatible with existing RRF3 configurations.

Also includes a bug fix for the CurrentLoopTemp sensors.

ajquick opened this pull request in dc42/RepRapFirmware

closed Allow Chip-Select Pins to Be Used by More than One Sensor. #383

dc42

Thanks, but I won't be able to consider your PR until development on 3.02 has started, because 3.01 is close to final now.

ChrisP

I hadn't really followed this topic much until I got a github notification this morning, but I'm kinda hyped about this now!

One of the systems I work on has a requirement of having to read from a significant number of thermistors, and following a conversation with @T3P3Tony a while ago, he suggested using a MCP3208 (it'll actually need 2).
Anyway, I've only just realised that your current loop sensors have already done pretty much all the groundwork - thanks
I have some MCP3208 here, so I'll knock up a board today and get testing!

Ultimately, I think my application would be better as a standalone CAN expansion board in the end as each sensor is monitoring an individual heater... but that's a next step for some future time.