Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin order as build options #120

Closed
wants to merge 3 commits into from

Conversation

peoyli
Copy link

@peoyli peoyli commented Nov 6, 2022

Added build_options.h file to set pin order at build time

@nielsonm236
Copy link
Owner

First: This is a great idea!
I'll need to assess the code size and RAM impact (although it looks like there is no additional RAM required).
On initial review I think my concern is that most users do not have the ability to compile code for their application. So I've tried to make options available via the GUI so that I don't have to produce and publish a large number of binary files for users to sort through. So I wonder if the concept you developed could be implemented and selected via the Feature options in the GUI. Unfortunately that would be a lot more Flash and would require a few bytes of RAM. So maybe not the right path.
Hmmmmm ... so maybe building more binaries is the better choice. Perhaps I need to come up with a more automated way to build the files, and a naming schema so that users can find what they need for their specific hardware implementation. The number of steps I have to go through to produce the binaries is already error prone.
Mike

@peoyli
Copy link
Author

peoyli commented Nov 6, 2022

As you noticed, my additions in the code does not require any more RAM or EEPROM space, but having the option to map the pins from the EEPROM-writable configuration would be the best way (probably later), or as you mention, build multiple binaries for each of the firmware variants.
Also, the idea behind adding the build_options.h file, is that it could hold the build options for production builds from the uipopt.h file, so they will be easier to find and even automatically change when building for different relay board pinouts.

@nielsonm236
Copy link
Owner

This is coming up next on my task list. I will probably copy your code into one of my branches rather than do a merge (mostly because I'm not sure how to get the merge to work right). But I want to be sure you get credit for this. I will think a bit on the best way. Perhaps I can complete my current release, then merge your code, then make any needed modifications before my next release.

@nielsonm236
Copy link
Owner

I've been looking at this more. My comments:

  1. While I understand the "build_options.h" approach I already do this in "uipopt.h", and have updated that a bit to make my own build options method easier for me to operate ... now that so many build options are needed for a lot of different reasons. So, I want to omit that aspect of the contribution.

  2. I think I will make the pinout selection a "URL" based command since it is something rarely needed,, and usually it will only be set one time for a given module. Thus GUI selection is not essential. On the other hand, having some indication in the GUI that shows which pinout is being used would be helpful. Making a selection via the URL also eliminates the need for a build option. Perhaps adding "selection" to the GUI could be done later if we find the URL method is inadequate.

I'll make sure you have credit in the manual. Do you know if I can make you visible as a contributor in Github without a "Merge" of code from you?

@peoyli
Copy link
Author

peoyli commented Jan 22, 2023

I've been looking at this more. My comments:

  1. While I understand the "build_options.h" approach I already do this in "uipopt.h", and have updated that a bit to make my own build options method easier for me to operate ... now that so many build options are needed for a lot of different reasons. So, I want to omit that aspect of the contribution.

If this will not be a build option (as described in '2'), then go ahead and implement this as a changeable setting. I was thinking the same way - that it only need to be set once for a module/project and that it would save ROM space by doing it at compile time.

  1. I think I will make the pinout selection a "URL" based command since it is something rarely needed,, and usually it will only be set one time for a given module. Thus GUI selection is not essential. On the other hand, having some indication in the GUI that shows which pinout is being used would be helpful. Making a selection via the URL also eliminates the need for a build option. Perhaps adding "selection" to the GUI could be done later if we find the URL method is inadequate.

It's a good idea to be able to see the pin order in the GUI, could be as simple as those lines of ASCII-art I provided in the build_options.h file, or just "pin-order: 1 (see manual)". Likewise having a drop-down list to select the pin-order would not consume that much ROM space (and it could redirect to the URL command for setting the order).

I'll make sure you have credit in the manual. Do you know if I can make you visible as a contributor in Github without a "Merge" of code from you?

Don't know if that is possible, only found that "collaborators" could be added, but I'm too unfamiliar with the terms Github uses

@nielsonm236
Copy link
Owner

Re: Adding Collaborator
I think I figured out how to do this. When I add your code and prepare the merge from my branch it looks like I have the option of adding a Co-Developer that will then add you as a Collaborator. I will try that and hopefully it will show you as a Contributor in Github.

@nielsonm236
Copy link
Owner

I have this code mostly complete and have run a few basic tests. Looks good so far. I need to push my current release out, then I will complete development and testing on this and get it released. I ended up placing all options in Flash then I let the user select which pinout they want. I have not built the user interface yet. The UI will have to be very light weight, and by that I mean only tens of bytes added to the existing UI.

@nielsonm236
Copy link
Owner

Now that I have this in test there is a fairly significant issue. Obvious once I saw it.
Issue: If Physical pin 16 is being used for DS18B20 it must not be remapped.
Issue: If Physical pins 14 and 15 are being used for the I2C bus they must not be remapped.
Issue: If Physical pin 11 is being used for the UART it must not be remapped.
This can be handled with firmware, but ends up being a good deal more expensive from a Flash perspective.
I will have to think this through:

  • Whatever I come up with needs to fit in the Flash available in the non-upgradeable versions and BME280 versions ... or it becomes a feature not available in those versions.
  • If a pin cannot be remapped, what is done with the other pins? For instance, if pin 16 cannot be remapped to pin 8, what IO is mapped to pin 8? This could end up being very chaotic from the user perspective.

The concept here is good if all of the pins just drive relay boards. But if any of the pins are used for alternative functions (DS18B20, I2C bus, UART) then the concept falls apart.

Thinking out loud: So maybe "Relay Only" versions could be built with the alternative pin-mapping function, but those builds will not support DS18B20, I2C bus, and UART. This means no temperature sensors, no ethernet upgradeability, no future I2C functionality, and the debug UART is not available. (although the UART issue could be worked around in the development environment).

@peoyli
Copy link
Author

peoyli commented Jan 26, 2023

Is there any light-weight way of checking is pins have been reserved for I2C, UART and DS1820 ? Then just omit switching those pins (when in use), or even more simple:
Disable pin order setting when any of the reserved pins are in use. This will allow a 8-channel relay board to be connected to the lower pin row.

@nielsonm236
Copy link
Owner

"Is there any light-weight way of checking is pins have been reserved for I2C, UART and DS1820 ? Then just omit switching those pins". I don't think it would be light-weight due to the fact that the "IO to Pin" mapping is a const struct in Flash. So for each exception pin I would have to over-ride the pin map for that pin. I will look at this more closely but don't think it will work. And it might be confusing to the user: Let's say I have selected the pin mapping for an 8 pin module, but I also have DS18B20 devices on physical pin 16. If I can't remap physical pin 16 it will be connected to the DS18B20 and to the relay on that physical pin.

"Disable pin order setting when any of the reserved pins are in use. This will allow a 8-channel relay board to be connected to the lower pin row." Disabling pin order setting when any alternative pin-mapping is selected would be easy. But the second part of your statement makes be wonder why we bother with the alternative pin mapping for 8 channel relay boards ... with the current mapping they could already just connect to the lower pin row. So maybe that mapping is not needed?

From this I'm trending toward "If a reserved pin is in use disable pin mapping". We could still keep the 8 pin option in case it is useful to someone (only costs about 40 bytes of Flash). This would allow the functionality to be added without additional builds (a GREAT simplification), providing alternative mapping to users that are "Relay usage Only". The Manual will have to carefully explain all this so that users are very careful to understand how this works with their target relay connectivity. Some users will still have to make their own hardware interface boards, but the firmware mapping could make some of the problem a little easier to manage.

Thoughts?

@nielsonm236
Copy link
Owner

Mostly a note to myself: I we use the above described approach the one remaining "complication" is the Debug UART. It is on IO11 / Physical Pin 11 (if enabled), so if that IO relocates to another pin use of the UART will need to take that into consideration. I don't think this is a problem as debug needing the UART can be done with the original mapping, then users can pick whatever mapping they want.

@nielsonm236
Copy link
Owner

Another note to myself: If this weren't such a tiny environment I could copy the desired mapping to RAM ... then if I had exception pins I would have more flexibility to replace specific-pin-mapping where there is reserved pin usage, eliminating the need for reserved pin interpretation at multiple points in the code. It would still be difficult for a user to understand what is happening. But it might work. The problem right now is the map would need 32 bytes of RAM ... and in some builds we don't have that. Keep this back-of-mind as a potential opportunity.

@peoyli
Copy link
Author

peoyli commented Jan 28, 2023

You should be able to use only 16 bytes of RAM (and EEPROM) for each of the pin mappings, but it might still be too much for the available RAM (and would require more code in the flash for shifting and masking the value of each byte to get the from-to pair)
I haven't investigated the code enough to see what's going on, but with all the changes since I made this pull request, I will be looking at old code which might or might not be so useful going forward.

Flash space for code: how will you see if it will fit ?
RAM usage: how can you measure the usage, or debug the code once running on the controller ?
(both are probably answered if I read through all documentation for STM8)

Looking forward to going on with actual testing (for my application of the controller, and 16 port relay board connected to it) - you added the most needed enhancement in December (set all ports with one command), but setting the correct pin order would also be helpful in my case (because no 16-channel relay board have the same pin mapping as the HW-584).

@nielsonm236
Copy link
Owner

Re: "You should be able to use only 16 bytes of RAM (and EEPROM) for each of the pin mappings," Not enough RAM available to do this.

Re: "Flash space for code: how will you see if it will fit ?" My "before I do anything" guess is just based on experience. But as I develop code I can see the impact on Flash in IdeasSTM8 by right-clicking the "NetworkModule.sm8" file, clicking "Inspect Object" and adding up the .vector, .const and .text sections, then subtracting 0x7c0. This tells me how much Flash is left. I need to leave at least 500 bytes (prefer 1000) to allow future debug for bug fixes ... and of course the space consumed by a bug fix. If I'm lucky there is a lot of space left for features ... but there hasn't been much space left lately. The builds that are the most limited are the MQTT (non-upgradeable) and BME280 builds.

Re: "RAM usage: how can you measure the usage ..." RAM usage is found the same way as Flash usage except I add up the .data, memcpy_update, .bss and .iconst sections. For example in the MQTT upgradeable build this adds up to 1514 bytes of RAM used ... out of 1536 bytes available (per the chip spec).

Re: "or debug the code once running on the controller?" I can enable a UART transmit function on Pin 11 to display checkpoint print statements on PuTTY. In the code you'll see a few "UARTprintf" statements left behind from prior debug. They are commented out or contained within #if #endif statements that remove them from code for production builds. I usually leave some behind in case there is a post-release bug found ... as new bugs are typically in areas recently worked on. Every now and then I clean them out.

I think I have the code for Issue #120 ready for deep testing. My initial surface level testing is all good so far. I'm adding in code for Issue #115 so they can be released together. That code is ready for initial surface level testing.

Mike

@nielsonm236
Copy link
Owner

In my last reply on Re: "Flash space for code: how will you see if it will fit ?" I said "then subtracting 0x7C0" I meant to say "0x7c80". But "0x7C80" applies to Upgradeable Code builds. Code builds for Non-Upgradeable code have a ceiling of 0x7e80 (an additional 512 bytes). The ceiling is set by the reserved space for Browser IO Names and Timers, and (for upgradeable builds) the reserved space for the I2C Driver and copy_ram_to_flash() function. The Manual section "Developers: Flash Memory Map" has more detail.

@peoyli
Copy link
Author

peoyli commented Jan 30, 2023

For the build of the firmware with pin-order setting (web, non-upgradeable):
32384-(128+9624+17218) = 5414 bytes free (that's plenty of space, but could probably be optimized with more reusable strings for the web pages).

My thought on RAM available for use.. 512 bytes is reserved (by default) for the stack, and you have a check in 'main' for stack overflow. In that loop, it could be useful to store the minimum value of the stack pointer as a debug value for the statistics page (/66) to find out how much (if any) you can reduce the stack and reclaim valuable bytes of RAM.

@nielsonm236
Copy link
Owner

I think you'll find you built the Browser Non-Upgradeable. If you build the MQTT Non-Upgradeable the amount of available Flash is down to about 1000 bytes. We need about 500 bytes to run any debug. So that leaves about 500 for code changes.

@nielsonm236
Copy link
Owner

nielsonm236 commented Jan 30, 2023

With regard to the stack, that would be worth revisiting. Some history:

  • First of all, the stack overflow check is really there to catch errant pointers, which I've had trouble with from time to time as the use of small buffers in an IP environment gets a bit tricky.
  • The part that needs investigating again is whether (and how) the stack size can be changed. I spent a lot of time investigating this a couple of years ago, but in the end I must admit how to modify stack size was just something I could not understand. I've attached the User Guide for the compiler I'm using.
  • Once I realized I personally could not figure out how to move the stack boundary from the default I got creative with use of the stack space. My memory is vague on all the issues, but the two big ones I recall are;
    a) The POST parsing function required a large temporary buffer, much larger than any memory left in RAM. I allocated 300 bytes on the stack for this buffer, and of course once the function completes the memory is freed up. You can search on "local_buf" to find it.
    b) I have some recollection that a RAM buffer may cross the RAM/Stack boundary when the Code Uploader processes are transferring I2C EEPROM contents to Flash. At that point a small amount of code is loaded into RAM and run from RAM ... as the Flash is being written and I can't run the code from Flash at that time. This one truly is vague ... I may have only been considering crossing the boundary.

If you look at the NetworkModule.map file there is a Stack Usage section that is handy to see what functions consume the stack. Likewise there is a Symbols section that shows what variables and constants are stored in RAM and Flash.

CXSTM8_UsersGuide.pdf

nielsonm236 added a commit that referenced this pull request Mar 7, 2023
#115 "How to enhance the needed pins for sensors? Can the PCF8574 module be useful?"

#120 "Pin order as build options"

#141 "Selectable pinout branch"

#142 "Added REST command /50 (at /80)"

#143 "Linked pins check in check_runtime_changes() should be in check_eeprom_settings()"

#144 "periodic_service() function is running too often"

#145 "Timer error has gradually increased - need improved time keeping function"

#146 "Incorrect content-length for most pages returning content"
@nielsonm236
Copy link
Owner

Addressed in 20230305 1816

@nielsonm236 nielsonm236 closed this Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants