brewpi / firmware Goto Github PK

Makes it easier for newcomers to know where to put source code and where to look to find existing sources.

Also an overview of the main folder structure in the root readme.md would be useful.

While refactoring temperature conversions I discovered that many of the conversions were not rounded, but instead just truncated in a divide.

In other occasions the sign of the value was not taken into account:
The now correct implementation for division is in all temperature conversion functions is:

int8_t rounder = (rawTemp < 0) ? -25 : 25;
rawTemp = (rawTemp * 90 + rounder) / 50; // round result

To clarify: add half the divisor when positive -half when negative.

Results have been checked by unit tests.
For Fahrenheit temperatures, there can still be 1 bit errors, because the result is rounded in string to fixed point version first and later scaled to internal temp format in Celcius (*5/9). The error is max 1 bit (1/512 degree).

The actuators in BrewPi are acting on fridge temperature. When this temperature is not available, it goes into IDLE.

We could create a new class, called FallBackSensor, which tries to read one sensor, and if it is unavailable, reads a second sensor.

This is a straightforward way to implement heating/cooling based on the beer sensor, when the fridge sensor is unavailable.

It will allow using beer profile mode with just one sensor and it will create extra redundancy for our normal use case with a fridge and a beer sensor available.

Line 636 of BrewPi.py calls bg_ser.stop and fails when programming a blank Arduino using the Legacy branch.

Looking at the code, bg_ser is of type None and does not get defined on line 374 unless there is a detected hwVersion.
So right now its only possible to Re-Program Arduinos that have an existing hwVersion and trigger that code to create the BackGroundSerial instance, if the hwVersion is none because its a new Arduino its impossible to flash.

Some additional logic is required i think to check in the ProgramArduino/ProgramController case to ensure bg_ser is not null.

Currently reserved object size is sent over serial when an object is created. This is risky and puts a responsibility on the client it shouldn't have.

The firmware knows the maximum size an object can become (calculated based on proto files in case of a protobuf definition). It should reserve this size in EEPROM.
This will simplify the protocol because the size can be removed from these commands in request and response:

• Write (system) value
• Create object
• Read (system) value
• List objects payload
• Log values payload

For objects that have variable size, the size can be part of the payload and this can still be indicated with size 0.

The alarm can be enabled by sending 'A' over serial en disabled by sending 'a'.
However, when this is done on the current version, the sound is not a nice continuous beep.

It sounds like the pin is turned on and off very quickly.

Since we plan to release at least every 2 weeks, and automated release process will ease the burden of doing that plus ensure consistency.

At a minimum the release build should:

• pull the latest state from the master branch
• fetch the version information from the working copy
• tag the repo with the version number (if necessary, force pushing the tag)
• perform a clean build of the controller app
• rename the artefact to include the version number - brewpi-1.2.3.bin
• push the artefact to the github releases page

I have lots of parts laying around, including Arduino protoshields and DS18B20 sensors that I wanted to build my own shield for this legacy version. However, I am completely confused what schematics 0.2.10 is intended for. I've found different configurations where the temp sensors are on A4/5 and others where it's on D11/12, and even stacked on A4! Which is it?

Once I have this setup, I'd love to help development of this legacy version but am really having trouble getting the hardware in the right place.

I've also seen interest from others.

The strings that are received over serial for setpoints and other settings are not error checked.

atol just returns 0 on error, which is indistinguishable from atol("0").

By using strtol instead, we can check the end pointer after the function and handle parse errors.

When I build the files locally, I get:
target/controller-photon/firmware.elf
target/controller-photon/firmware.bin

When the files are built by Travis, the logs show that the build tires to make a controller.bin instead of a firmware.bin file and some things go wrong with the CRC handling.

When doing a release, I manually rename the files to:
brewpi-0.4.2-photon.bin

I think it would be great that when building, the file name would be:
appname-version-platform.bin

Errors on Travis:

~/build/BrewPi/firmware/platform/spark ~/build/BrewPi/firmware
/home/travis/build/BrewPi/firmware/platform/spark
building PLATFORM=core APP=controller
source path /home/travis/build/BrewPi/firmware
using 0.4.2-53-gb3025a6 as build name
stat: head: cannot open `/home/travis/build/BrewPi/firmware/platform/spark/target/controller-core/controller.bin.pre_crc' for reading: No such file or directory
cannot stat `/home/travis/build/BrewPi/firmware/platform/spark/target/controller-core/controller.bin.pre_crc': No such file or directory
head:    text      data     bss     dec     hex filename
cannot open `/home/travis/build/BrewPi/firmware/platform/spark/target/controller-core/controller.bin.pre_crc' for reading: No such file or directory
 108312    2208   10172  120692   1d774 /home/travis/build/BrewPi/firmware/platform/spark/target/controller-core/controller.elf
✓ SUCCESS

The binary generated by travis cannot be used, because dfu-suffix is not working correctly.

The automatically deployed files to GitHub releases do not work and need to be replaced by locally compiled working binaries.

We need to investigate why dfu-suffix doesn't work and fix.

Controlbox was initially written with nested objects in mind, to which one could write data in a nested ID. Smart objects would be implemented as containers themselves, which each value having an ID at the nesting level.

This overlaps with protobuf as our chosen communication protocol for most objects, which has tagged fields and nested messages.
Keeping the nested IDs complicates walking the object tree and processing data on the service side and makes the firmware more complex to maintain.

If we switch to a global object list, which can be a simple std::vector, adding objects, finding objects, removing objects is all much simpler and can use safe std implementations. We don't need custom container walkers and other custom object management implementations.
If we do want to write to a nested ID inside an object, the stream parser of that object can handle that. It doesn't require supporting nesting through the entire object chain.

I propose a simpler global root container, which keeps an std::vector of object entries:

struct ObjectEntry {
   uint16_t object id,
   uint16_t object type,
   uint8_t active_in_profiles,
   unique_ptr<Object> obj // ptr to runtime object
};

A unique pointer is used to not allow other objects to reference an object directly. The should get the runtime pointer from the global list from the id each time they access it. This ensures objects die if removed from the list, no dangling objects. Failed lookups will get a safe backup object if not found (for example a sensor always returning an error temp).
The object id does not need to be stored in the objects anymore, neither the type or active profiles.

An object lookup just iterates over the vector. This is probably plenty fast enough. If not, a map could be used.
Inactive objects (no runtime instance) that are defined in EEPROM but not loaded can be added to the list. Their unique ptr will be nullptr.

This allows the firmware to quickly list which objects exist but are not loaded. The global list can be created during startup.

Objects will not store their EEPROM location. They can request a one-time use writer/reader from the EEPROM manager based on their ID. This allows the EEPROM manager to reshuffle/defragment the EEPROM if needed. The EEPROM writer finds the object in EEPROM and rewrites it, in the same or a new location.

With the Spark Core being WiFi enabled, it makes sense to leverage this to get Wireless brewpi.

• setup wifi credentials.
  • via Serial: the Web UI will accept the WiFI details and send these to the device via the script. This requires a new command in the uC to accept and set the wifi credentials using the api WiFi.setCredentials().
  • Using listening mode and the Spark app should also be an option. (The Web UI has a button that when clicked puts the device in listening mode to save users from having to open the case.)
• is the uC a TCP client or server?
  • as a client: will have to continually monitor the connection, and reconnect when down. How to configure the IP to connect to? mDNS/zeroconf would avoid the need for configuration.
  • as server: no IP config needed on the spark, but on the rpi, unless zero conf is used. Also no need for the spark to continually reconnect.
  • use zeroconf to avoid IP config
• brewpi.py discover command line param will list discovered uC devices. each device given with it's device ID. Default ID is MAC address. This is copied by the user into brewpi config to talk to a specific device?
• how is the brewpi script configured to connect to either serial or wifi?
  • startup attempts to look on serial and broadcasted devices on mDNS
• How does the device know which interface to use? There is only one global piStream object which will talk either over serial or WiFi. It may need to be a multiplexer object - route to serial and to any TCP connections. (this is what was done in the nice-firm repo - the code relies on the boost library but should be relatively easy to back port to the current brewpi code.)

I noticed an issue running tests with my new fridge.

To reduce the number of compressor cycles, I set the minimum ON time for the compressor at 10 minutes and the period at 60 minutes. This caused the fridge temperature to fluctuate above the setpoint, instead of around it.

This fridge responds very quickly to compressor action, so what will happen is:

1. During the 10 minutes of cooling, the fridge temp drops quickly.
2. This reduces proportional gain (because temp approaches setpoint). So during a PWM cylce, the actuator value changes. Due to the time limits and long period, the actuator cannot follow the quicly changing actuator value.
3. The achieved actuator action is fairly constant, but the actuator setting changes quickly.
4. And this is a guess: Because the actuator achieved value does not reach the setting, anti windup is triggered.

I have added a simulation to mimic this behavior, see below.

The current implementation of object type is a uint8 with a value of 0-127 to define the application object type.
This is a small range that could be exhausted quickly.
While it is unlikely that we will have more than 127 concurrent object types on the firmware, it leaves no room for deprecating objects.

If we change the implementation to use 2 bytes instead of one, we have a much bigger pool so we can do version management of objects:

• If we change the implementation of an object in a way that the previous object definition is not compatible, we can assign it a new ID. The service layer can read the old definition (which is still stored in EEPROM) and create an object of the new type with the same settings for those that are compatible.
• If we deprecate an object, we do not have to re-use the object ID for a new type, which would be dangerous.

The object creation command now uses an array of factories that is directly indexed by the object type ID. Instead of using the object ID directly as the index, we should search the array for the matching factory.

The current color scheme was proof of concept. The colors are ok, but I feel they don't harmonize well.

Would be nice to rework the scheme so we have

• colors that work well together
• colors that subtly help describe the device state

The present UI design ensures colors are not the only indicator - information conveyed by color is accompanied by some other indicator (usually text)

Reported by user TomA:

During startup after power on, the RPi 2 seems to send an EEPROM reset command to the BrewPi Spark.

When the Pi reboots while the power is kept on, this does not happen. Only when the power is shortly interrupted to the Raspberry Pi and the BrewPi.

I think the commands to reset EEPROM is to simple, being just 'E'. It is too easily sent accidentally.

As reported on the community here:
https://community.brewpi.com/t/controller-debug-message-warning-2-temperature-sensor-disconnected-pin-0/499/8

It seems that the sensor never truly disconnects, but it fills the logs a disconnect.

Strange is that it seems much more prevalent for the room temp sensor. I have investigated this earlier and confirmed that even when sensors are swapped, the room temp sensor sees more disconnects, but not exclusively. We need to investigate why.

What's the difference between an .hex and a .bin file? In the documentation, it is said to use the latest .hex available, but, since the latest .hex provided is for 0.2.10, maybe the documentation is old and I'm supposed to upload the .bin ones, instead.

I have my brewpi spark set to °F. When I enter a value in the "Maximum difference between fridge and beer set point (= output of PID)" field then click "update from core" the value returned is my entered value multiplied by 9/5 or 1.8.

As reported here:
https://community.brewpi.com/t/temperature-sensor-high-temp-at-startup/1476

When a temperature is read from a DS18B20 sensor that is not correctly initialized (triggered a temp conversion), it can read a temperature of 85C.

We should recognize this error and make sure the data is not used.

Newly flashed devices will not store user settings without first having the eeprom initialized.

Currently, if the version stored in eeprom doesn't match the version expected by firmware, the eeprom is used in read-only mode - settings are not stored. (We are missing notification to the user about this, but that's another issue.)

When the version is 0xFF (uninitialized) then the system will perform an eeprom reset to the current version. This ensures that settings will be stored from first use.

Example:

Mode   Off
Beer   23.75 --.- °C
Room   23.5  --.- °C
Idling for   5h24m17

To reproduce the error:

• In advanced settings, set absolute maximum temp to 110.
• Set beer temp to 100.5
• Set beer temp back to 20
• Beer set point now shows as 20.05 on web ui LCD

Scenario: the user can alter temperature setpoints and the mode using the embedded UI

Feature: user can initiate mode selection
Given brewpi running in non-test mode
When the user taps the mode region
Then a popup dialog of available modes appears.

Feature: mode selector
Given the user has initiated mode selection
When the user clicks on a mode,
Then the mode is set
And when the user clicks outside the dialog,
Then the mode is not changed

Feature: user can initiate setpoint selection
Given brewpi running in non-test mode
When the user taps a temperature box
Then it changes to a temperature input box

Feature: user can change the setpoint
Given a temperature input box is visible
When the up arrow is pressed,
Then the temperature increases by the current temperature step, initially 0.1 degrees
And When the down arrow is pressed
Then the temperature decreases by the current temperature step

Feature: user can increase the temperature step
Given a temperature input box is visible
When the left arrow is pressed,
Then the temperature step is multiplied by 10, limited to a maximum of 10

Feature: user can decrease the temperature step
Given a temperature input box is visible
When the right arrow is pressed,
Then the temperature step is divided by 10, limited to a minimum of 0.1

The script continuously updates the setpoint on a ramp, but the controller shouldn't log each update, this is too verbose in the logs:

Aug 02 2017 19:18:00 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:00 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:01 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:02 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:02 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:03 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.15
Aug 02 2017 19:18:03 Controller debug message: INFO MESSAGE 12: Received new setting: beerSet = 51.

When creating an object we will allow not defining an ID in advance but letting the controller assign one. The controller has to return the resulting ID as part of the response so we can reference the object afterwards.

Current implementation has downsides:

• Objects belong to only one profile. If a sensor is used in multiple profiles, it is repeated.
• Objects can only be added to last profile in EEPROM
• Only one profile can be active at any time (doesn't match well with multiple processes)

New proposal:

• Profiles do not contain objects, but objects have a flag (uint8 bitfield) for which profiles they are active. This defines 8 profiles.
• A global bitfield defines which profiles are active.
• Activating a profile constructs runtime objects from EEPROM definition
• Deactivating a profile deletes runtime objects if they are not assigned to a still active profile

Advantages:

• The object list is one global list. No segmentation of EEPROM in multiple sections. No notion of profiles open/closed for adding objects.
• Existing objects can be assigned to one or more profiles
• Profiles can be activated/deactivated at will

Hi,

Reporting the issue we discussed. The temperature set was not respected by the controller. That's the 3rd time it happened so I guess that's really a bug.

The system is running with a Valve board and a valve configured as a cooler.
When I rebooted the controller, everything went back to normal, so it does not seem to be a hardware related problem.

I'm attaching the stderr log and the graph to show the problem.

stderr_bpi.txt

When FreeRTOS was added to the repo,
Some files where unintentionally overwritten in commit d506f63

There readme was later removed instead of restored in commit:
7a48947

Bugreport from DanCook:

I had Kp set to 100 when I first observed this, and have been setting it to other large positive numbers to see if there is some logic in the repro. (reason so high: I have a 30-watt mat heater trying to take on 40 gallons of water, admittedly underpowered, and I was trying to cause oscillations around the setpoint so that I could figure out ultimate gain).

Here is a screencast of a repro: https://goo.gl/2UK5pn1. In the video I set Kp to 200, verify in the algorithm window and in the log, then come back to advanced settings. If I click "Update from Core" the value has gone negative again. When Kp = 200 the associated negative value = -56, and if Kp = 400 the negative value = -112 (so ironically the behavior is proportional!)

The JSON value in the Control Algorithm tab says the expected value, even when the Advanced Settings box shows a negative value.

We should get back into writing unit tests and to do this we need to add a test framework to the build. We previously used googletest and it worked well.

Each OneWire actuator has a DS2413 or DS2408 member (device), which is a stateless class and their interface to the actual hardware.

The actuator classes cache the device state to prevent frequent reads and writes. The actual device is shared between two valves or two output pins in hardware, but the software classes each keep a separate cache. This leads to reliability problems, because not all reads and writes go through each cache.

It would be a better design to refactor the hardware interface member out of the actuator classes and use a reference instead. Then the two classes can share the same cache.

It should be investigated what causes this. Is the value in EEPROM invalid? Is it not stored correctly or not read correctly?

This is a problem when there is no physical access to the controller: while in the calibration screen, the controller cannot be controlled remotely. We could add a timeout, with a warning over serial.

To ensure that communication and storage errors do not cause faults, we need to add CRC checks.

For communication:

• A request ends in an 8 byte CRC. Requests with an invalid CRC are not processed and an error code will be returned.
• A response ends in a CRC. This allows the service to handle communication errors.

This can be implemented by letting the DataIn and DataOut communication streams calculate a running CRC.

Objects in EEPROM will be stored ending with a CRC value. This makes the CRC of object definition + CRC zero. Listing multiple objects therefore does not change the running CRC value.
We can:

• Check the CRC per object and remove invalid objects if encountered
• When listing all objects, skip CRC calculation for streaming of the object itself that already is postfixed with a CRC. The total CRC of the message will still be valid, but when an object with invalid CRC was encountered, the complete message fails. The next time it will succeed because the object is removed.

When the script sends 'null' or 'None' to the controller, the temperature is set to 0.0, it should be set to INVALID_TEMP instead (which is shown as --.- on the screen.

Hi guys,

Firmware 0.5.5 seems to render JSON with a syntax error which causes brewpi-www and others to be unable to read device lists or, when a list can be read, cause unexpected crashes in parsing of values.

The culprit:

"Log2TempT

T:{"BeerTemp":5.56,"BeerSet":2.00,"BeerAnn":null,"FridgeTemp":7.25,"FridgeSet":-4.00,"FridgeAnn":null,"Log1Temp":28.88,"Log2TempT:{"BeerTemp":5.56,"BeerSet":2.00,"BeerAnn":null,"FridgeTemp":7.25,"FridgeSet":-4.00,"FridgeAnn":null,"Log1Temp":28.88,"Log2Temp,"State":4}
                                                                                                                                ^
                                                                                                                                Bad stuff happens here.

I'm going to attempt reverting to the firmware just before this one - but I think you should make this a high priority issue since automatic firmware upgrades appear to receive 0.5.5 when you perform them today (May 20th 2018).

EDIT: please note that there is another occurrence of the same quoting error later in the example string above. It's where the property name Log2TempT gets rendered a second time.

When the proportional gain is very low (which could be used with a PID driving another setpoint), it is possible to not reach setpoint in steady state.

This is due to a combination of events.
This can only happen if the actuator is not reaching setpoint, by margin a. This margin is multiplied by the anti-windup gain of 3. If p equals 3a, we are stuck.

The integrator is increased with p every cycle.
The antiwindup is (pid result - achieved output) * 3.
When these two cancel each other out, the integrator is stuck in a steady state value, without the error being zero.

Currently the screen updates are slow. This is for 2 reasons:

• each pixel set carries overhead
• the clock rate is lower than it could be due to the touch screen

This issue will address the first problem - reducing per-pixel overhead.

The most frequent method called is D4DLCD_Send_PixelColor_ili9341 to add one more pixel within the defined window. It results in 2 calls to D4DLCDHW_SendDataWord, which itself asserts/deasserts the Data Control line and then performs an SPI transfer, which asserts/deassers the Chip Select line, then calls SPI.transfer(). With many pixels sent back to back, that is considerable overhead that is unnecessary.

One way to speed this up is to buffer consecutive SendDataWord requests to a memory buffer, and then write this buffer as one SPI transaction. The buffer is written out when

• when full
• when a SendCommandWord request is received
• when a D4DLCD_FlushBuffer_Spi_Spark_8b is called, indicating the current object rendering is complete.

By using DMA and 2 memory buffers, we can transfer the data to the display asynchronously from one buffer while the other buffer is being filled with new data.

We should have a /docs folder in the firmware repo, for the firmware specific developer documentation.

ReadTheDocs should build from this docs folder to make it easy to navigate and read.

There are some constants that are presently hard-coded that ideally should be configurable under advanced settings:

• minimum time between heating/cooling
• minimum heating/cooling time

(If there are others, please add to the list)

These could be made into settings and added to the settings structure stored in eeprom, and given corresponding settings attributes in the json encoding over serial.

If I set my minimum on time, send to core, then update from core to verify setting, it appears correct. However, making any other changes, such as max difference or Beer-to-Fridge proportional gain (Kp), then clicking update from core, resets the min on time to zero.

fixedPointToString used longTempDiffToInt, to get the integer part of the string. This function however, rounds the result.

When it later rounds the fraction part (0.998) to a whole integer, this integer is added twice. The result is converting 100.998 to 112 instead of 111.

While testing the new fallback sensor, I noticed that the inputError on a PID is not set to invalid on invalid input. It will slowly go to a negative value instead.

The PID should update these values to indicate that it does not get valid input and it should update its variables and output accordingly.

The protobuf spec defines that default values are omitted from a message: if the receiver does not receive a value, it is assumed to be default.

I implemented sparse updates by only overwriting values in the object that are present in the message, but this is wrong.
In the protobuf spec, there is no way to distinguish an omitted member from a member that is set to its default value.

A possible workaround is to set the default value to something outside of the normal data range.
This complicates the design because:

• We deviate from normal protobuf use
• We have 'defaults' that are different from the actual default values of the values.
  After discussion, we decided that we will remove the sparse update mechanism in its current form and an update to an object will write the entire object.

To update a single nested value the client can read the object, change a value and write the object back.

A future implementation to support sparse updates in a way that is compatible with the protobuf spec is to include a field mask in the message. The field mask, when present, indicates which values should be copied from the message.

What's preventing this device from working for mashes? I saw a mention on the websites that mashing wasn't supported yet - though the firmware would be updated later.

In the current controlbox implementation, object IDs are not part of the creation command or object definition.
They are simply the (nested) position/index of the object in the container hierarchy, which is determined when the object is instantiated.

If an object is deleted, it is replaced by a null pointer, which is fine.
But the next time the device reboots and the objects are recreated and placed in the container, they will have a different ID.

This will make it impossible to let objects use ID's to cross-reference each other.
To get truly fixed IDs, this solution is proposed:

• Make the container ID part of the object creation command
• When the slot for the ID in the object definition does not exist yet, create it by adding new containers and expanding them to get the desired number of slots.

To allow the controller to assign an object ID, we should reserve an ID value to indicate 'next free slot'. The service wants to create (multiple) objects async. Issuing a 'get free slot' command and a creation command for that specific slot will cause race conditions because it needs to happen atomically. Reserving a special ID value to indicate 'next free slot' simplifies the design because it moves the atomic sequence (get free slot and assign object to it) to the controller.

To reproduce:

• Install a beer sensor in device manager
• Remove power form controller
• Unplug beer sensor
• Power on controller and wait for it to boot
• Plug in beer sensor
• Beer temperature now swings from very high to very low

The cause of this bug:
Temperature sensor filters could be used uninitialized due to a bug in the failedReadCounter: when a fridge or beer sensor was plugged in 1-60 seconds after start, the filter was used with uninitialized data and returned invalid temperature.

Fixed in: 3c19c1a

I noticed today that if the current integral value is set to something, say 20, and you create a new profile and start cooling, the integral value never climbs backs to zero while the state is not idle. In my situation the temp error is greater than iMaxError, and since the fridge target temp is so low (it's attempting to cool 8 degrees), it takes a while to reach an IDLE state, which means it's still using the integral value of '20' in it's PID calculation, which throws off the target fridge temp. I would expect the integral value to just be zero since the error is great than iMaxError.

I forked the code and don't mind fixing it. I'm just curious if this is how I should fix it:

If abs(error) > iMaxError, then no matter what decrease the value by 1/8 (even if the state is not idle).