{"action":"closed","issue":{"url":"https://api.github.com/repos/bxparks/AceButton/issues/13","repository_url":"https://api.github.com/repos/bxparks/AceButton","labels_url":"https://api.github.com/repos/bxparks/AceButton/issues/13/labels{/name}","comments_url":"https://api.github.com/repos/bxparks/AceButton/issues/13/comments","events_url":"https://api.github.com/repos/bxparks/AceButton/issues/13/events","html_url":"https://github.com/bxparks/AceButton/issues/13","id":361072603,"node_id":"MDU6SXNzdWUzNjEwNzI2MDM=","number":13,"title":"proposed deprecation of AdjustableButtonConfig into ButtonConfig","user":{"login":"bxparks","id":26933282,"node_id":"MDQ6VXNlcjI2OTMzMjgy","avatar_url":"https://avatars0.githubusercontent.com/u/26933282?v=4","gravatar_id":"","url":"https://api.github.com/users/bxparks","html_url":"https://github.com/bxparks","followers_url":"https://api.github.com/users/bxparks/followers","following_url":"https://api.github.com/users/bxparks/following{/other_user}","gists_url":"https://api.github.com/users/bxparks/gists{/gist_id}","starred_url":"https://api.github.com/users/bxparks/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bxparks/subscriptions","organizations_url":"https://api.github.com/users/bxparks/orgs","repos_url":"https://api.github.com/users/bxparks/repos","events_url":"https://api.github.com/users/bxparks/events{/privacy}","received_events_url":"https://api.github.com/users/bxparks/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-09-17T22:47:34Z","updated_at":"2018-09-30T19:57:35Z","closed_at":"2018-09-30T19:57:35Z","author_association":"OWNER","body":"Hello everyone,\r\n\r\nThanks for using the `AceButton` library. I am thinking of making a change to the `ButtonConfig` class of the library, and was wondering if anyone has any strong opinions.\r\n\r\n# Background\r\n\r\nWhen I first created the AceButton library, I was aiming to have a low RAM requirement for the library. Coming from the cloud computing world (where I have access to GiB and TiB of RAM), it was difficult to wrap my head around a programming environment where I have only 2kB of RAM (ATmega328P processor on an UNO or Nano). I made `ButtonConfig` mostly immutable, containing compile-time constants that seemed reasonable for various timing parameters, and used `virtual` functions to dispatch down to the `AdjustableButtonConfig` subclass to allow people to override those parameters if they wanted to.\r\n\r\n# Proposal\r\n\r\nFor the next revision of the AceButton library, I am proposing to merge the `AdjustableButtonConfig` class into the `ButtonConfig` class, and remove most of those `virtual` methods that retrieve those timing parameters. The `AdjustableButtonConfig` class will be kept for backwards compatibility, but it will become just a empty shell subclass of `ButtonConfig` with no additional features. I can remove that class in later versions.\r\n\r\n# Reasons\r\n\r\n1) In theory, making `ButtonConfig` adjustable should increase the RAM usage by 12 bytes (because there are 6 `uint16_t` timing parameters in `AdjustableButtonConfig` which are not present in `ButtonConfig`). But in practice, I have found that the compiler optimizes the code in strange ways, and the overal RAM usage does not actually increase in many cases.\r\n\r\n2) Removing the `virtual` method on the various getters allows the compiler to inline those methods, instead of using a virtual dispatch, and makes the `AceButton.check()` method run faster. Of course, we are talking about a difference between, say, 17 microseconds versus 14 microseconds, so maybe this is micro-optimizing, but I think a simple button library should have as little overhead as possible.\r\n\r\n3) Removing `AdjustableButtonConfig` makes the code simpler, easier to maintain, easier to document, and hopefully easier to use.\r\n\r\n# Benchmarks\r\n\r\nHere are some benchmarks. I used Arduino IDE version 1.8.6, using the `arduino:avr:nano:cpu=atmega328old` fully-qualified board identifier. Other processors have so much more RAM than the ATMega328P, (e.g. 64kB for Teensy 3.2, 80kB for ESP8266, ~300kB for ESP32), that we will only worry about RAM size for the ATMega328P family of boards.\r\n\r\n* **before** means the current code, \r\n* **after** means the new code in the `adjustable` branch where I have merged `AdjustableButtonConfig` into `ButtonConfig`.\r\n\r\n## CPU Time\r\n\r\n### AutoBenchmark\r\n\r\nbefore:\r\n```\r\n------------------------+-------------+---------+\r\nbutton event            | min/avg/max | samples |\r\n------------------------+-------------+---------+\r\nidle                    |  16/ 16/ 24 | 1925    |\r\npress/release           |   8/ 18/ 28 | 1920    |\r\nclick                   |   8/ 17/ 28 | 1922    |\r\ndouble click            |   8/ 16/ 32 | 1921    |\r\nlong press/repeat press |   8/ 19/ 28 | 1919    |\r\n------------------------+-------------+---------+\r\n```\r\nafter:\r\n```\r\n------------------------+-------------+---------+\r\nbutton event            | min/avg/max | samples |\r\n------------------------+-------------+---------+\r\nidle                    |  12/ 13/ 20 | 1934    |\r\npress/release           |   8/ 14/ 20 | 1925    |\r\nclick                   |   8/ 14/ 24 | 1925    |\r\ndouble click            |   8/ 13/ 24 | 1925    |\r\nlong press/repeat press |   8/ 15/ 24 | 1927    |\r\n------------------------+-------------+---------+\r\n```\r\n\r\n## Program Size and RAM Usage\r\n\r\n### HelloWorld\r\n* before (flash/ram): 2406/55\r\n* after (flash/ram): 2282/55\r\n\r\n### SingleButton\r\n* before (flash/ram): 3812/234\r\n* after (flash/ram): 3692/234\r\n\r\n### StopWatch\r\n* before (flash/ram): 5962/268\r\n* after (flash/ram): 5686/256\r\n\r\n### TunerButtons\r\n* before (flash/ram): 4348/306\r\n* after (flash/ram): 4364/318\r\n\r\n### ClickVersusDoubleClickUsingBoth\r\n* before: 2342/55\r\n* after: 2218/55\r\n\r\n### CapacitiveButton\r\n* before: 5488/266\r\n* after: 5498/298\r\n\r\n# Summary\r\n\r\nFor the most part, the new code consumes less flash memory, but surprisingly, it often uses the same amount of static RAM.\r\n"}}
{"actor":{"display_login":"bxparks"}}